-
Notifications
You must be signed in to change notification settings - Fork 55
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
feat(components/sidepanel): accept custom action ids #846
feat(components/sidepanel): accept custom action ids #846
Conversation
action.id || | ||
action.label | ||
.toLowerCase() | ||
.split(' ') |
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.
a regexp could be used here but i guess it is a lot more readable the way you did it
action.label | ||
.toLowerCase() | ||
.split(' ') | ||
.join('-'); |
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.
Please move this to an actionId generator.
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.
✅
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.
Can you add an entry in breaking changes log please ?
The id was ignored before, not now, it can break QA tests. This should be documented
What is the problem this PR is trying to solve?
Actions ids are base on the action's label. It cause problems with i18n (for example TDP onboarding uses theses IDs to target elements).
What is the chosen solution to this problem?
Actions can have a new
id
property.Please check if the PR fulfills these requirements
[x] This PR introduces a breaking change
Before this, action ids were ignored. Now, if an id is provided per action, it will be used instead of the label ; so it could break some QA tests.