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

New dropdown menu top nav bar #6854

Conversation

ramitaarora
Copy link
Member

@ramitaarora ramitaarora commented May 14, 2024

Fixes #2455

What changes did you make?

  • Created a hoverable dropdown menu in the top navigation
  • Fixed the links in _data > navigation > main.yml according to new design
  • In _data > navigation > main.yml, added link-name variable
  • Fixed the hover states for the desktop navigation and new dropdown menu

Why did you make the changes (we will use this info to test)?

  • I made these style changes in accordance to the new design in figma
  • I added the link-name variable in order to have the dropdown menu render dynamically, rather than hard-coding each filename.

Screenshots of Proposed Changes Of The Website (if any, please do not screen shot code changes)

Visuals before changes are applied

image

Visuals after changes are applied Screenshot 2024-05-14 at 12 24 44 PM

Copy link

Want to review this pull request? Take a look at this documentation for a step by step guide!

From your project repository, check out a new branch and test the changes.

git checkout -b ramitaarora-new-design-top-nav-bar feature-homepage-launch
git pull https://github.com/ramitaarora/hfla-site.git new-design-top-nav-bar

Note that CONTRIBUTING.md cannot previewed locally; rather it should be previewed at this URL:

https://github.com/ramitaarora/website/blob/new-design-top-nav-bar/CONTRIBUTING.md  

@github-actions github-actions bot added role: front end Tasks for front end developers Complexity: Large Status: Updated No blockers and update is ready for review P-Feature: Navigation Feature: Feature Branch Requires Branching off a Feature Branch instead of gh-pages size: 3pt Can be done in 13-18 hours labels May 14, 2024
@LRenDO
Copy link
Member

LRenDO commented May 15, 2024

Hi @marioantonini and @roslynwythe! When you have a minute, please add your ETA and Availability. Thanks!

@marioantonini
Copy link
Member

  1. Availability: W, Th, Fr: 2-4
  2. ETA: EoD 5/16/2024

Copy link
Member

@marioantonini marioantonini left a comment

Choose a reason for hiding this comment

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

The links under Toolkit are not resolving properly, but I guess that is an issue with the actual toolkit page not having the proper anchor tags. Some of the navigation links seem wonky (meaning they go inconsistently to either same page or different pages), but that is beyond the scope of this particular issue.

Apart from the requested changes involving classes and ids, the navigation looks good!
All the dropdowns work properly. Links are behaving as expected. And everything looks as nice as the figma files!!

_includes/main-nav.html Outdated Show resolved Hide resolved
_includes/main-nav.html Outdated Show resolved Hide resolved
_includes/main-nav.html Outdated Show resolved Hide resolved
_sass/components/_header-nav.scss Outdated Show resolved Hide resolved
_includes/main-nav.html Outdated Show resolved Hide resolved
_sass/components/_header-nav.scss Outdated Show resolved Hide resolved
_sass/components/_header-nav.scss Outdated Show resolved Hide resolved
@marioantonini
Copy link
Member

After discussing with @ExperimentsInHonesty, the conclussion is that adding the anchor tags to the Toolkit page is part of the scope of this issue.

@ramitaarora please add the proper anchor tags to the file toolkit.html

_includes/main-nav.html Outdated Show resolved Hide resolved
@ramitaarora
Copy link
Member Author

Hey @roslynwythe and @marioantonini , I made the requested changes! For the #our-work anchor tag, I added it to the homepage under projects, but let me know if I should put it in a different place.

As discussed in office hours, I left the toolkit links as is, since that seems to be a bigger issue. :)

@roslynwythe
Copy link
Member

roslynwythe commented May 25, 2024

@ExperimentsInHonesty

  1. There was no named anchor target for the link "Our Story". Just to prove that navigation is working, an anchor was added on the homepage just above the heading "Explore our projects by Program Area".
  2. On the Toolkit page, the named anchors for Guides and External Resources do exist, however the links are not working, and we believe that is a result of the manipulation of the url (via window.history.replaceState) that is performed within Toolkit.js, as part of the filter implementation. For example, if you enter the url /toolkit#guides in the browser, the address is reset to /toolkit/? and the portion of the url after the "#" is lost. I believe this will require modification to the javascript and that may not fall within the scope of this issue. Please advise.
  3. There is an accessibility issue: the new navigation bar is not fully navigable via keyboard. A keyboard user can navigate between the top level menu items but is not able to expand the drop down menu. Please advise, is this within the scope of this issue?

@ExperimentsInHonesty
Copy link
Member

@roslynwythe

  1. There was no named anchor target for the link "Our Story". Just to prove that navigation is working, an anchor was added on the homepage just above the heading "Explore our projects by Program Area".

I don't see a menu item "Our Story" on the menu example of the figma layout
image

  1. On the Toolkit page, the named anchors for Guides and External Resources do exist, however the links are not working, and we believe that is a result of the manipulation of the url (via window.history.replaceState) that is performed within Toolkit.js, as part of the filter implementation. For example, if you enter the url /toolkit#guides in the browser, the address is reset to /toolkit/? and the portion of the url after the "#" is lost. I believe this will require modification to the javascript and that may not fall within the scope of this issue. Please advise.

@ramitaarora Please make an ER for this

  1. There is an accessibility issue: the new navigation bar is not fully navigable via keyboard. A keyboard user can navigate between the top level menu items but is not able to expand the drop down menu. Please advise, is this within the scope of this issue?

@roslynwythe @ramitaarora the accessibility is within the scope of this issue.

@roslynwythe
Copy link
Member

@ExperimentsInHonesty I apologize, I meant to write that there was no named anchor target for the link "Our Work" on the homepage. Just to prove that navigation is working, an anchor was added on the homepage just above the heading "Explore our projects by Program Area".

@ramitaarora please contact me on Slack if you need help writing the ER or with implementing keyboard navigation for the drop down.

@ramitaarora
Copy link
Member Author

@roslynwythe I am working on the keyboard accessibility - which js file could I write this code in? Or should I make a new one?

@marioantonini
Copy link
Member

  1. Availability: Th, Fr: 2-5
  2. ETA: EoD 6/7/2024

@marioantonini
Copy link
Member

marioantonini commented Jun 7, 2024

Changes look good. I tested the navigation, and apart from the discussed issue with the links in Toolkit, everything is pointing to the right place.
@roslynwythe @ramitaarora the only thing that for some reason I was not able to test properly was keyboard navigation. But that might have been a problem on my end.

Copy link
Member

@roslynwythe roslynwythe left a comment

Choose a reason for hiding this comment

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

@ramitaarora great job adding assets/js/nav-accessibility.js for keyboard navigation. However in mobile and tablet views, while the "hamburger" menu icon does expand via keyboard, the menu options themselves are not reachable via keyboard navigation.

Copy link
Member

@roslynwythe roslynwythe left a comment

Choose a reason for hiding this comment

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

Great job @ramitaarora

  • code changes are correct and clean
  • PR is well-formed and descriptive
  • changes check out in the browser

There are some changes we would like to see in the menu before the homepage launch, but we have decided to accept this PR as-is and to create new ERs/issues to address these changes. These changes are:

  • remove hyperlinks from the top-level menu items
  • improvement to keyboard navigation to use down arrow and/or Enter for submenu
  • remove Toolkit menu item from header menu
  • remove Toolkit item from footer menu

@roslynwythe roslynwythe merged commit 45dd205 into hackforla:feature-homepage-launch Jun 19, 2024
@github-actions github-actions bot added Ignore: Test Issue was created for testing purposes only and removed Status: Updated No blockers and update is ready for review ready for product labels Jun 22, 2024
@t-will-gillis t-will-gillis removed the Ignore: Test Issue was created for testing purposes only label Jun 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Complexity: Large Feature: Feature Branch Requires Branching off a Feature Branch instead of gh-pages P-Feature: Navigation role: front end Tasks for front end developers size: 3pt Can be done in 13-18 hours
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants