-
Notifications
You must be signed in to change notification settings - Fork 64
margin fix of the heding FILTER BY inside the filters menu #1931
Conversation
the top and bottom margins should be 32px
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for working on this, @anton202!
The VSearchGridFilter
component is used in several places: in the sidebar and in the modal, both with the old_header
designs (that you can see on the Audio Release Figma mockup) and on the new_header
designs (that we are currently implementing under a feature flag, and you can see here).
In each place, the VSearchGridFilter
component has different padding. So, it is best to add the padding in the parent components instead of the component itself.
Could you please remove the padding values from section
here, and add the following values:
<VSearchGridFilter @close="onTriggerClick" /> |
This line should become:
<VSearchGridFilter class="px-10 py-8" @close="onTriggerClick" />
And this line:
<VSearchGridFilter @close="onTriggerClick" /> |
Should be class="px-6 pt-[7px] pb-10"
to match the mockups.
class="px-10 pt-8"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for fixing it so quickly, @anton202! This works really well for the old_header
design, which you can currently see on wordpress.org/openverse.
Could you also add changes for the new_header
version?
To turn it on locally, you will need to go to http://localhost:8443/preferences
and check the new header
checkbox there. Then, you will see the site with a different header design. And the desktop sidebar filters there now have no padding :) Could you add the same padding to them:
<VSearchGridFilter @close="toggleSidebar" /> |
And also, remove the now-unnecessary !p-0
from the modal version here:
openverse-frontend/src/components/VHeader/VHeaderMobile/VContentSettingsModal.vue
Line 44 in a8842fb
class="!p-0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The #filters-heading
<h4>
has a much taller line height (leading-8
) compared to the mockups (130%). This is causing the padding to appear more than it actually is. I added the spaces of and they only add up to 24px, the rest being incalculable height added due to line height.
Reducing the line height to the expected value and increasing the padding will ensure the correct 32px padding.
I believe that the line height was added to prevent the jumping of the filters when the filters are selected, @dhruvkb, and the 'Clear filters' button appears. The button must have enough paddings to be accessible and tappable. I think it would be easier to fix it in a separate PR by making the button absolutely positioned. This way, it wouldn't affect the positioning of other elements. |
the top and bottom margins should be 32px
Fixes
Fixes #1581 by @panchovm
Description
cahnged the top and bottom margin of the heading: FILTER BY to match the margins in the design
screenshot
Checklist
Update index.md
).main
) or a parent feature branch.Developer Certificate of Origin
Developer Certificate of Origin