-
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: add new props to feature flag props #17239
fix: add new props to feature flag props #17239
Conversation
✅ Deploy Preview for v11-carbon-react ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for carbon-elements ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
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.
Looks great, thanks for adding tests! Just a couple questions
const parentScope = useContext(FeatureFlagContext); | ||
const [prevParentScope, setPrevParentScope] = useState(parentScope); | ||
|
||
const combinedFlags = { | ||
'enable-use-controlled-state-with-value': enableUseControlledStateWithValue, |
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.
It doesn't look like this is actually used anywhere anymore, is that what you see as well? The useControlledStateWithValue in here doesn't seem to be used anywhere either
carbon/packages/react/src/internal/FeatureFlags.js
Lines 49 to 51 in 36685a0
export const useControlledStateWithValue = enabled( | |
'enable-use-controlled-state-with-value' | |
); |
If so we should remove this one
enableUseControlledStateWithValue, | ||
enableV12TileDefaultIcons, | ||
enableV12TileRadioIcons, | ||
enableV12Overflowmenu, | ||
enableTreeviewControllable, | ||
enableExperimentalFocusWrapWithoutSentinels, |
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.
Do these need the same defaults as what's defined in the root flag definition?
carbon/packages/feature-flags/feature-flags.yml
Lines 25 to 52 in b9abee7
- name: enable-experimental-tile-contrast | |
description: > | |
Enable the experimental tile improved contrast styles | |
enabled: false | |
- name: enable-v12-tile-default-icons | |
description: > | |
Enable rendering of default icons in the tile components | |
enabled: false | |
- name: enable-v12-tile-radio-icons | |
description: > | |
Enable rendering of radio icons in the RadioTile component | |
enabled: false | |
- name: enable-v12-overflowmenu | |
description: > | |
Enable the use of the v12 OverflowMenu leveraging the Menu subcomponents | |
enabled: false | |
- name: enable-treeview-controllable | |
description: > | |
Enable the new TreeView controllable API | |
enabled: false | |
- name: enable-v12-structured-list-visible-icons | |
description: > | |
Enable rendering of radio icons in the StructuredList component | |
enabled: false | |
- name: enable-experimental-focus-wrap-without-sentinels | |
description: > | |
Enable the new focus wrap behavior that doesn't use sentinel nodes | |
enabled: false |
Closing in favor of #17266, I moved my review comments to there as well |
Closes #17238
Add new boolean props for all existing feature flags
Update FeatureFlag internals to map the boolean props back into an object form to be used in the scope of FeatureFlagContext
Changelog
New
-Update FeatureFlag internals to map the boolean props back into an object form to be used in the scope of FeatureFlagContext
Testing / Reviewing