Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix default UI for dropdown and mega menu #2644

Merged
merged 13 commits into from
Jun 5, 2023
Merged

fix default UI for dropdown and mega menu #2644

merged 13 commits into from
Jun 5, 2023

Conversation

lougoncharenko
Copy link
Contributor

PR Summary:

Mega Menu

  • Applied Bold Font to L2 Links
  • All links have the same size
  • Applied 30px padding to the top and bottom and between rows in the menu
  • Mega Menu opens below L1 link

Dropdown

  • Menu has a vertical padding of 20px on the top and bottom
  • When L3 links are dropped down, there is a 20px padding on the top and bottom
  • Indent space is removed on L3
  • L3 links have a hover and clicked state

Why are these changes introduced?

Fixes #2579

What approach did you take?

Made L2 links bold by default in the stylesheet
Made all links the same 1.4rem size by default
Applied a padding of 3rem for the mega menu's vertical padding and applied 3rem to the grid gap

added a hover effect on L3 links by applied a text-decoration (underline)
kept the original gray background on the L3 links for visual clarity.

Visual impact on existing themes

Not applicable as these are default settings

Testing steps/scenarios

  • Test all menu types (dropdown & megamenu)
  • Add more L3 links to both menus to ensure styling doesn't change
  • Test with gradient and color schema.
  • Test in mobile and desktop view

Demo links

Checklist

@lougoncharenko lougoncharenko self-assigned this May 16, 2023
@lougoncharenko lougoncharenko linked an issue May 16, 2023 that may be closed by this pull request
[Header] Fix default UI for dropdown and mega menu #2579
Closed
9 tasks
@ludoboludo ludoboludo self-requested a review May 16, 2023 16:09
ludoboludo
ludoboludo reviewed May 16, 2023
View reviewed changes
Copy link
Contributor

@ludoboludo ludoboludo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good! I've found one issue with the positioning of the mega menu and I have a few other questions 🙂

assets/base.css Outdated Show resolved Hide resolved
assets/component-mega-menu.css Show resolved Hide resolved
assets/component-mega-menu.css Outdated Show resolved Hide resolved
lougoncharenko
lougoncharenko commented May 17, 2023
View reviewed changes
assets/base.css Outdated Show resolved Hide resolved
@YoannJailin
Copy link

YoannJailin commented May 17, 2023 •
edited
Loading

Hello @lougoncharenko, thanks for the changes!

For the dropdown :

  • Do you think we could have paddings closer to what we can see in the figma? I was refering to 20 pixels everywhere but maybe setting this in the code doesn't gives the same visual result. I've added a version of the dropdown without the purple paddings so it's a bit more clear.
  • I agree the underline is too thick on Hover+Active, can we keep the same thickness as before?

@lougoncharenko
Copy link
Contributor Author

Per Yoann, waiting on Dan's approval to glance over and approve design choice before merging branch

@YoannJailin
Copy link

@lougoncharenko one last comment!

I feel like the spacing indicated in purple is still a bit to high. And the blue one just a bit too small.
Group 7939

Is there a way we can tweak them a bit more so they feel visually equal? Intended result would look something like this :
2

@lougoncharenko
Copy link
Contributor Author

@YoannJailin , I made the vertical padding at 1.8 rem. Each link has a 0.6 padding top and bottom, so I figured the 0.6rem for the link above + 0.6rem for the link below + 1.8 vertical padding will be 3rem, which is what the padding top and padding bottom is for the mega menu container. It looks more visually appealing now with the changes, but in case it still looks off, let me now :)

ludoboludo
ludoboludo reviewed May 30, 2023
View reviewed changes
assets/base.css Outdated Show resolved Hide resolved
assets/base.css Outdated Show resolved Hide resolved
assets/component-mega-menu.css Outdated Show resolved Hide resolved
ludoboludo
ludoboludo reviewed May 31, 2023
View reviewed changes
Copy link
Contributor

@ludoboludo ludoboludo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One extra nitpick and I think it's looking good to go 🙂

assets/base.css Outdated Show resolved Hide resolved
ludoboludo
ludoboludo reviewed May 31, 2023
View reviewed changes
assets/base.css Outdated Show resolved Hide resolved
ludoboludo
ludoboludo approved these changes Jun 1, 2023
View reviewed changes
Copy link
Contributor

@ludoboludo ludoboludo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a niptick to remove something that I don't think is necessary anymore, otherwise looks good and ready to ship! 🎉

assets/component-mega-menu.css Outdated Show resolved Hide resolved
eugenekasimov
eugenekasimov approved these changes Jun 1, 2023
View reviewed changes
Copy link
Contributor

@eugenekasimov eugenekasimov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved 🚀 👍

I left a minor nitpicker 👇

assets/base.css Outdated Show resolved Hide resolved
@lougoncharenko
Copy link
Contributor Author

@eugenekasimov and @ludoboludo , I made the changes that were requested and just requested one more review before pushing the PR :) thank you for taking the time to review this PR

ludoboludo
ludoboludo approved these changes Jun 5, 2023
View reviewed changes
Copy link
Contributor

@ludoboludo ludoboludo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good 👍 Didn't catch anything funky

eugenekasimov
eugenekasimov approved these changes Jun 5, 2023
View reviewed changes
Copy link
Contributor

@eugenekasimov eugenekasimov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀

@lougoncharenko lougoncharenko merged commit 9a72773 into main Jun 5, 2023
5 checks passed
@lougoncharenko lougoncharenko deleted the louisa-fix-default-ui-for-dropdown-and-mega-menu branch June 5, 2023 16:31
ludoboludo pushed a commit that referenced this pull request Jun 5, 2023
@lougoncharenko @ludoboludo
fix default UI for dropdown and mega menu (#2644)
1a2683f 
* added vertical padding and made l2 bold to mega menu

* added vertical padding to drop down

* fixed default UI for dropdown and mega menu

* added hover effect to all l2 links and fixed overlapping or mega menu

* adding padding to l3 links and fixed hover effect for dropdown

* reverted the mega menu back to the current setting

* changed horizontal grid gap on mega menu and vertical paddings on dropdown menu

* made vertical grid gap at 1.8 rem to match the 3 rem vertical paddings on mega-menu container

* removed the unnecessary lines of code

* removed the text-thickness that was making l2/l3 links a different size from l1 links

* added a hover effect over active link

* removed uneccessary link size and white space
lougoncharenko added a commit that referenced this pull request Jun 7, 2023
@lougoncharenko
fix default UI for dropdown and mega menu (#2644)
9a31d16 
* added vertical padding and made l2 bold to mega menu

* added vertical padding to drop down

* fixed default UI for dropdown and mega menu

* added hover effect to all l2 links and fixed overlapping or mega menu

* adding padding to l3 links and fixed hover effect for dropdown

* reverted the mega menu back to the current setting

* changed horizontal grid gap on mega menu and vertical paddings on dropdown menu

* made vertical grid gap at 1.8 rem to match the 3 rem vertical paddings on mega-menu container

* removed the unnecessary lines of code

* removed the text-thickness that was making l2/l3 links a different size from l1 links

* added a hover effect over active link

* removed uneccessary link size and white space
pangloss added a commit to pangloss/dawn that referenced this pull request Jun 14, 2023
@pangloss
Merge remote-tracking branch 'shopify/main' into next
bb855f2 
* shopify/main: (59 commits)
  [Announcement bar] Add social icons (Shopify#2497)
  Update theme version to match the pubic release (Shopify#2698)
  Add release/v10.0.0 branch fixes to main (Shopify#2693)
  fix default UI for dropdown and mega menu (Shopify#2644)
  Fix link formatting in Related Products heading (Shopify#2680)
  Update 2 translation files (Shopify#2637)
  Enable gift card recipient form by default on featured product section (Shopify#2666)
  Gift cards/enable recipient form by default (Shopify#2618)
  Add a Color Scheme setting for Menus-Header (Shopify#2622)
  Made mobile drawer full width by default-header (Shopify#2625)
  Allow multiple announcement bars in Header group (Shopify#2619)
  [Feat Product] Add rating styling sheet (Shopify#2620)
  Fix password page variables (Shopify#2607)
  Fix transform applied when it should not for sliders (Shopify#2606)
  Modify info string for gift card recipient checkbox (Shopify#2588)
  [3D lift animation] Raise hovered card above adjacent cards (Shopify#2589)
  Remove fallback color scheme info text (Shopify#2602)
  Fix CSS specifity issue to cancel animation for theme editor events (Shopify#2605)
  Remove settings daya for icon color (Shopify#2601)
  Follow ups for accessibility of announcements slider (Shopify#2580)
  ...
lougoncharenko added a commit that referenced this pull request Jun 21, 2023
@lougoncharenko @qui-gon-jon
@ludoboludo @sofiamatulis @eugenekasimov @krzksz @LucasLacerdaUX @kjellr @shopify @kmeleta
made the test/default blog post 3 by default (#2670)
48ef65f 
* made the test/default blog post 3 by default

* blog default posts are not mobile responsive and on a slider. created snippet for blog-placeholder that renders into featured-blog with changable arguments

* removed commented out code

* default blog posts now are modifiable by post-limit and column number presets

* reformated code, reverted header back to h1, removed snippet, and made changes as suggested

* noticed the blog section needs a margin bottom when color scheme is applied

* removed padding from blog and removed date and author presets

* Fix link formatting in Related Products heading (#2680)

* fix default UI for dropdown and mega menu (#2644)

* added vertical padding and made l2 bold to mega menu

* added vertical padding to drop down

* fixed default UI for dropdown and mega menu

* added hover effect to all l2 links and fixed overlapping or mega menu

* adding padding to l3 links and fixed hover effect for dropdown

* reverted the mega menu back to the current setting

* changed horizontal grid gap on mega menu and vertical paddings on dropdown menu

* made vertical grid gap at 1.8 rem to match the 3 rem vertical paddings on mega-menu container

* removed the unnecessary lines of code

* removed the text-thickness that was making l2/l3 links a different size from l1 links

* added a hover effect over active link

* removed uneccessary link size and white space

* Add release/v10.0.0 branch fixes to main (#2693)

* Fix color for image in password (#2608)

* [Image behavior] fixed background jitter fix on mobile (#2611)

* [Animation in editor] Remove transform translate when re rendering and re ordering blocks (#2614)

* Remove transform translate when re rendering and re ordering blocks

* target only what's needed

* [bug] Horizontal scrolling on mobile.  (#2617)

* Add overflow-x:hidden to prevent horizontal scrolling on mobile.

* Change margin for slider for mobile and tablet.

* Remove redundant preconnect to cdn.shopify.com (#2621)

* Update reverse scheme (#2626)

* [Footer] Remove Global Media settings inheritance from images (#2631)

* Send timezone offset as string instead of integer (#2636)

* Fix slider scrolling issue (#2635)

* Fix background gradient for Related Products. (#2641)

* Update from Shopify for theme dawn/release/10.0.0

Committed from shop: Skeleton Theme

* Quick add remove animation from image and content (#2657)

* Add default values for color scheme group (#2660)

* Revert unwanted changes (#2669)

* Update from Shopify for theme dawn/release/10.0.0

Committed from shop: Skeleton Theme

* Revert "Update from Shopify for theme dawn/release/10.0.0"

This reverts commit 89d927e.

* Remove forced white text color in Image Banner & Slideshow (#2663)

* Remove forced white color.

* Change color scheme for Dawn's default homepage Image Banner..

* Remove warning about color scheme usage.

* Fix bug with transparent buttons on mobile.

* Remove unused info key from translated files.

* Update display of button when it's in a mobile container.

* Update existing placeholder images (#2610)

* Update existing placeholder images

* Featured collection and product card

* Featured product section

* Collection list section

* Featured blog and multirow sections

* Slideshow: change order of placeholder images

* Collage adjustments for product and collection cards

* Image with text adjustments

* Featured product: re-add `.product--no-media` selectors

* Update featured product section

* Update `card-collection`

* Cleanup `card-collection`

---------

Co-authored-by: Ludo <ludo.segura@shopify.com>

* Gradient fix for transparent background medias and cards (#2651)

* Fix media, product, and collection cards placeholder (#2682)

* Fix missing else for collection cards (#2692)

---------

Co-authored-by: Sofia Matulis <sofiamatulis@users.noreply.github.com>
Co-authored-by: Eugene Kasimov <105315663+eugenekasimov@users.noreply.github.com>
Co-authored-by: Mateusz Krzeszowiak <mateusz.krzeszowiak@shopify.com>
Co-authored-by: Lucas Lacerda <37168033+LucasLacerdaUX@users.noreply.github.com>
Co-authored-by: Kjell Reigstad <kjell@kjellr.com>
Co-authored-by: shopify[bot] <79544226+shopify[bot]@users.noreply.github.com>
Co-authored-by: Ken Meleta <30790058+kmeleta@users.noreply.github.com>
Co-authored-by: Andrew Etchen <andrew.etchen@shopify.com>

* made the test/default blog post 3 by default

* added updated placeholder image

* added the different placeholder images, while maintaining ability to edit with presets

* blog card theme settings work on default blog posts

* removed css that wasn't being used

* reverted to original code without theme settings

* fixed prettier error by removing extra quotation mark and made test blog default instead of news

---------

Co-authored-by: Jon Neill <jonathan.neill@shopify.com>
Co-authored-by: Ludo <ludo.segura@shopify.com>
Co-authored-by: Sofia Matulis <sofiamatulis@users.noreply.github.com>
Co-authored-by: Eugene Kasimov <105315663+eugenekasimov@users.noreply.github.com>
Co-authored-by: Mateusz Krzeszowiak <mateusz.krzeszowiak@shopify.com>
Co-authored-by: Lucas Lacerda <37168033+LucasLacerdaUX@users.noreply.github.com>
Co-authored-by: Kjell Reigstad <kjell@kjellr.com>
Co-authored-by: shopify[bot] <79544226+shopify[bot]@users.noreply.github.com>
Co-authored-by: Ken Meleta <30790058+kmeleta@users.noreply.github.com>
Co-authored-by: Andrew Etchen <andrew.etchen@shopify.com>
@kimberlyoleiro kimberlyoleiro mentioned this pull request Jul 12, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ludoboludo ludoboludo ludoboludo approved these changes

@eugenekasimov eugenekasimov eugenekasimov approved these changes

@kmeleta kmeleta Awaiting requested review from kmeleta

@YoannJailin YoannJailin Awaiting requested review from YoannJailin

@danielvan danielvan Awaiting requested review from danielvan

Assignees

@lougoncharenko lougoncharenko

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Header] Fix default UI for dropdown and mega menu
5 participants
@lougoncharenko @YoannJailin @danielvan @ludoboludo @eugenekasimov