-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
fix(content-switcher): call onChange with correct data with keyboard #6257
fix(content-switcher): call onChange with correct data with keyboard #6257
Conversation
Deploy preview for carbon-elements ready! Built with commit 5c43d8c |
Deploy preview for carbon-components-react ready! Built without sensitive environment variables with commit 5c43d8c https://deploy-preview-6257--carbon-components-react.netlify.app |
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.
is this technically a breaking change? related #6162
@emyarod good point, was thinking about that after seeing that proposal today. I think technically it is, but the previous behavior is wrong so I wasn't sure what the right way forward may be. |
I think we just need to treat it as a breaking change (related previous incidents in the website and design language website carbon-design-system/carbon-website#119 carbon-design-system/design-language-website#273) |
So is this being postponed until v11? |
Maybe the best question to ask here is if someone could use the as-is to implement a correct content switcher, or if it'd be too hard for the typical case? It seems like it'd be challenging to use the incorrect behavior here with a code switcher, maybe you'd need to pass in The reason this comes up is that if it's unreasonable for folks to implement/build a fix on top of the breaking behavior, then we could move forward with the change (since so no would be able to use it) but if there is an alternative way then we could rope this behind a feature flag, for sure. |
I think if we communicate the reason for the breaking change (the behavior was impossible/difficult to implement as-is) then I'm okay with it. |
Cool, @emyarod said something similar over in Slack too. I think we could make a case for this being a justified change given that the behavior is incorrect, and achieving the correct behavior is difficult. Let's go ahead and do that, I'll make sure to post a quick note too in our channel tomorrow. |
Is this something we want to start documetning in our code tabs. maybe at the bottom changes in x.x.x this was changed to get the behavior correctly? it seems like people never read our releases. Or as long as we communicate it in the release we can point them there ? |
bump @emyarod @tw15egan @aledavila when you get a chance 👀 |
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.
the change is fine. if we are pushing this ASAP we just need to be clear that it is a breaking change since this exact issue with content switcher has affected consumers like our own Carbon website multiple times (in the tickets I referenced)
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.
Also good with the changes since this is making the correct implementation difficult. It would be great if we make sure we have some messaging around this so we don't break users without letting them know!
When can we expect this to get in? |
Issue still persist |
Closes #6119
Changelog
New
Changed
onChange
with correct data when using keyboardRemoved