-
Notifications
You must be signed in to change notification settings - Fork 16
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
[Issue #1397] process roadmap accessibility #2392
[Issue #1397] process roadmap accessibility #2392
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a few questions, haven't tested locally yet.
Process: { | ||
milestones: { high_level_roadmap_items }, | ||
}, | ||
} = useMessages() as unknown as IntlMessages; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might just be a me thing, but I'm not sure I understand what this section is doing, can you explain it a bit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure!
As far as I can tell from the docs, useMessages
will always return all messages in use in the app. Since we only care about the high_level_roadmap_items
messages, I destructured the return value from the hook to dig into just that part of the object. That way we don't have to use a long chain to index into the object when we want to reference the high_level_roadmap items. Does that make sense? Happy to do a quick call to discuss
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for asking and for the explanation. It looks like we could ditch useMessages()
altogether and just iterate over the three roadmap items.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this whole thing is kind of odd IMO. My take is we probably shouldn't be directly using the translation messages to determine this functionality. I'd rather have a concept of the roadmap item keys outside of the translation and use those to create translated text.
On the other side of things, useMessages
seems to be created for the purpose of accessing next intl messages on the client side. Since this is a server component, we likely could access the messages directly as you're describing, but again, I think with a slightly bigger refactor that wouldn't be necessary. See https://next-intl-docs.vercel.app/docs/environments/server-client-components#option-3-providing-individual-messages
I'll take a crack at it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Verified it works locally, checked out upstream branch, enabled voiceover etc. so LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re-review post merge conflict
3bf3d73
to
02bb96e
Compare
Summary
Fixes #1397
Time to review: 20 mins
Changes proposed
Context for reviewers
Test steps
Additional information
note that requirement in the ticket specifying removal of "launch" aria text for the chevron icons seems to have already been fixed