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

Update menu html structure so it's one single hierarchical list #240

Merged
merged 4 commits into from
Jul 16, 2021

Conversation

colinbm
Copy link
Contributor

@colinbm colinbm commented Jun 24, 2021

⚠️ Don't forget to update the gem version in the CHANGELOG before merging! When you're ready to release bump version file and generate a tag. ⚠️

What

Updates the navigation to use a single top level ul and nested subnav.

Why

This is a follow up to #237 with a better solution.

The previous markup was like this:

<ul>
  <li>
    <a href="">Top level link</a>
  </li>
</ul>

<ul>
  <li>
    <a href="">Top level link</a>
    <ul>
      <li>indented nav</li>
    </ul>
  </li>
</ul>

This separates each top level nav item into a separate list, where the nav should be a single component.

New markup is:

<ul>
  <li>
    <a href="">Top level link</a>
  </li>
  <li>
    <a href="">Top level link</a>
    <ul>
      <li>indented nav</li>
    </ul>
  </li>
</ul>

@colinbm colinbm force-pushed the accessibility-menu-structure branch 2 times, most recently from bc73292 to cf838ac Compare June 28, 2021 16:18
@36degrees
Copy link
Contributor

I don't think b35334d belongs in this PR, but otherwise this seems to make sense to me.

@colinbm
Copy link
Contributor Author

colinbm commented Jul 5, 2021

@36degrees It's more adjacent... would you prefer it be in a separate PR? Or I can edit the title to include it explicitly?

@36degrees
Copy link
Contributor

I think it's more or less the same commit as f46aee7 in #238? At the minute it'll conflict with that PR because it's changing the same lines of code but in slightly different ways.

@colinbm
Copy link
Contributor Author

colinbm commented Jul 5, 2021

Ah so it is - thanks for clarifying - will remove it here.

@colinbm colinbm force-pushed the accessibility-menu-structure branch from cf838ac to 9c9c386 Compare July 5, 2021 14:08
@colinbm colinbm closed this Jul 7, 2021
@colinbm colinbm reopened this Jul 7, 2021
Copy link
Contributor

@36degrees 36degrees left a comment

Choose a reason for hiding this comment

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

Thanks @colinbm 👍🏻

@colinbm colinbm removed the request for review from richardTowers July 16, 2021 12:02
@colinbm colinbm merged commit 38d26fa into master Jul 16, 2021
@colinbm colinbm deleted the accessibility-menu-structure branch July 16, 2021 12:03
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.

2 participants