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
[Design2-108] DSCO - Add Aria/AriaDescribedBy attributes to components where needed #58469
Conversation
const ariaProps: {[key: string]: PrimitiveValue} = {}; | ||
Object.keys(props).forEach(key => { | ||
if (key.startsWith('aria-')) { | ||
ariaProps[key] = props[key]; | ||
} | ||
}); |
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.
this is fine and doesn't need to be changed, but I think it reads a little nicer with a fromEntries
and a filter
.
const ariaProps: {[key: string]: PrimitiveValue} = Object.fromEntries(
Object.entries(props).filter(([key]) => key.startsWith('aria-'))
)
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.
Agree it reads better in your suggestion. Though, I got interested and compared the perfomance of both implementation and the current one turned out to be faster.
The more props there'll be in an array - the bigger gap would be it was 12% average faster when there were ~15 props and it turned out to be 27% average faster when there were. ~25+ props pased. Giving that this function will be used a lot and the number of props passed here is unknown (it's possible there'll be 10 and more props for one component) I suggest we stick with current version. Reason of execution time difference smaller is a number of operations executed in the current version.
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.
minor non-blocking syntactic sugar suggestion, but otherwise lgtm 🚀
[Design2-108] DSCO - Add Aria/AriaDescribedBy attributes to components where needed
aria-
attributes support forButton
componentsaria-
attributes support for...Dropdown
componentsaria-
attributes support forFontAwesomeV6Icon
componentLinks
Testing story
Deployment strategy
Follow-up work
Privacy
Security
Caching
PR Checklist: