-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
RadioGroup: Log deprecation warning #68067
Conversation
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Flaky tests detected in a78d424. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/12380096509
|
# Conflicts: # packages/components/CHANGELOG.md
since: '6.8', | ||
alternative: 'wp.components.ToggleGroupControl', | ||
} ); | ||
if ( ! __shouldNotWarnDeprecated ) { |
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.
Adds a mechanism to suppress a confusing "double warning" coming from the ButtonGroup used internally in RadioGroup.
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.
React uses global variables for this purpose:
let didWarnAboutSomething = false;
function something() {
if ( ! didWarnAboutSomething ) {
didWarnAboutSomething = true;
console.error( /* ... */ );
}
}
They also use Set
s as didWarn*
values, when they want to track a warning for each component name or something else separately.
We could use this, too, instead of the semi-official prop.
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.
If we decide to go the didWarn
way, I think it makes sense to implement it directly in the deprecated()
helper function.
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.
I was thinking about smarter ways to implement this mechanism when I started logging the size deprecation warnings, but came to the conclusion that an explicit prop was the only way. I am imagining consumer code like this, where the ButtonGroup warning inside the RadioGroup should be suppressed, but the standalone ButtonGroup instance should still warn:
const MyUI = () => (
<RadioGroup />
<ButtonGroup />
);
I'm all ears if you have any clever ideas!
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.
Could component's internal context system help?
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.
Not in a way that's "more automatic" than the current manual prop-based system, I don't think. Also context could be problematic when render props or slots are involved (e.g. we don't want to suppress warnings on a component that the consumer passed in through a prefix
prop).
if ( ! __shouldNotWarnDeprecated ) { | ||
deprecated( 'wp.components.ButtonGroup', { | ||
since: '6.8', | ||
alternative: 'wp.components.__experimentalToggleGroupControl', |
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.
Added the __experimental
prefix for consistency.
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.
Tests well and code LGTM 👍
What?
Soft deprecates (#61099) the RadioGroup component.
Why?
All remaining instances of RadioGroup in the app have been removed as of #67702.
How?
This was already passive deprecated, so just escalating the deprecation so it notifies consumers through a console warning and Dev Note.
Testing Instructions
Go to the Storybook for RadioGroup and check that the deprecation warning is being logged in dev tools.
✍️ Dev Note
The
RadioGroup
component has been deprecated. To be consistent with the current WordPress design system, useRadioControl
orToggleGroupControl
instead.