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

Navigation JS fixes not picked up from upstream Protocol #14770

Closed
Tracked by #14773
alexgibson opened this issue Jul 1, 2024 · 5 comments
Closed
Tracked by #14773

Navigation JS fixes not picked up from upstream Protocol #14770

alexgibson opened this issue Jul 1, 2024 · 5 comments
Labels
a11y Issues related to accessibility Bug 🐛 Something's not working the way it should

Comments

@alexgibson
Copy link
Member

alexgibson commented Jul 1, 2024

Description

The main nav in bedrock is missing fixes such as mozilla/protocol#847 because it uses it's own (now outdated) copy of the nav JS.

I'm not sure what the best fix is. Maybe it's something we can address in the site refresh?

@alexgibson alexgibson added the Bug 🐛 Something's not working the way it should label Jul 1, 2024
@craigcook
Copy link
Member

hrm yeah, bedrock's nav is a customized component that never got backported to Protocol so all the names are different. We could implement the same fixes in bedrock's custom JS easily enough.

@alexgibson
Copy link
Member Author

I was wondering if we could actually revert to using Protocol's JS instead? It seems to be mostly duplicated just to change some selector names, and it seems like potentially a lot of effort to duplicate all the fixes?

@craigcook
Copy link
Member

Yeah, it was literally just to avoid naming conflicts. The redesigned nav restyled a bunch of things but the underlying structure is mostly the same, so I ended up dropping the mzp- prefix to avoid issues with core Protocol. Then had to dupe the script just to work with the different names. If we use Protocol's JS we'd need to add the prefix to the nav and try to avoid any styling conflicts as well, but that might not be too hard. Worth trying anyway.

@alexgibson
Copy link
Member Author

Possible resolutions:

  1. We update the JS in bedrock with the bug fixes that have landed in Protocol since the navigation was forked.
  2. We update the navigation class names / selectors in bedrock to match what the Protocol JS expects, and stop relying on the now outdated JS in bedrock.
  3. We finish back porting the navigation CSS in bedrock to Protocol, then publish an NPM update and bring everything back in.
  4. We fix this when the site refresh happens (it's still unclear if we'll use the existing navigation JS or start a-fresh?)

@alexgibson
Copy link
Member Author

This issue is being caught in the a11y checker job:

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a11y Issues related to accessibility Bug 🐛 Something's not working the way it should
Projects
Development

No branches or pull requests

2 participants