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.

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

LYNX-722: Style desktop navigation menu in boilerplate #307

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

Conversation

svera
Copy link
Collaborator

@svera svera commented Feb 7, 2025

Copy link

aem-code-sync bot commented Feb 7, 2025

Hello, I'm the AEM Code Sync Bot and I will run some actions to deploy your branch and validate page speed.
In case there are problems, just click a checkbox below to rerun the respective action.

  • Re-run PSI checks
  • Re-sync branch
Commits

Copy link

aem-code-sync bot commented Feb 7, 2025

Page Scores Audits Google
📱 / PERFORMANCE A11Y SEO BEST PRACTICES SI FCP LCP TBT CLS PSI
🖥️ / PERFORMANCE A11Y SEO BEST PRACTICES SI FCP LCP TBT CLS PSI

@svera svera changed the title Lynx 722 LYNX-722: Style the desktop navigation menu in boilerplate matching the mockups Feb 7, 2025
@sirugh
Copy link
Collaborator

sirugh commented Feb 7, 2025

@svera please resolve lint issues. If you cannot access the build failure log, let me know.

@svera svera changed the title LYNX-722: Style the desktop navigation menu in boilerplate matching the mockups LYNX-722: Style desktop navigation menu un boilerplate Feb 10, 2025
@svera svera changed the title LYNX-722: Style desktop navigation menu un boilerplate LYNX-722: Style desktop navigation menu in boilerplate Feb 10, 2025
Copy link
Collaborator

@loginesta loginesta left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @svera
Please see some comments below!

@@ -175,10 +175,20 @@ header nav .nav-sections ul > li > a {

header nav .nav-sections ul > li > ul {
margin-top: 0;
width: 300px;
Copy link
Collaborator

Choose a reason for hiding this comment

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

are we sure that it should be 300px? or should we use a variable/configuration?

}

header nav .nav-sections ul > li > ul > li {
font-weight: 400;
width: 300px;
Copy link
Collaborator

Choose a reason for hiding this comment

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

are we sure that it should be 300px? or should we use a variable/configuration?

position: absolute;
left: -24px;
width: 200px;
width: 900px;
Copy link
Collaborator

Choose a reason for hiding this comment

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

have you tested what happens if we have several items inside a category?
Screenshot 2025-02-10 at 12 33 39

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@loginesta how are you seeing all these items? I cannot see them in https://lynx-722--aem-boilerplate-commerce--hlxsites.aem.live/

Copy link
Collaborator

Choose a reason for hiding this comment

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

I have manually added them editing the DOM in order to see the behaviour of the Site Navigation in the browser

header nav .nav-sections ul > li > ul,
header nav .nav-sections ul > li > ul > li {
width: 100%;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am sure we need more styling for mobile using media queries.
it looks poor as it looks at the moment...
Screenshot 2025-02-10 at 12 31 07
Screenshot 2025-02-10 at 12 31 21

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants