Skip to content
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.

Sign up for GitHub

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

Jump to bottom

docs(featureflags): define naming convention, unify existing flag names #15964

Merged
merged 5 commits into from
Apr 3, 2024
Merged

docs(featureflags): define naming convention, unify existing flag names #15964

merged 5 commits into from
Apr 3, 2024
+112 −29

Conversation

tay1orjones
Copy link
Member

@tay1orjones tay1orjones commented Mar 14, 2024 •
edited
Loading

Closes #15728

This PR proposes a naming convention for feature flags and outlines an approach for how to utilize them with breaking changes in major version releases.

Changelog

New

  • Added enable-experimental-treeview-controllable as a replacement for enable-treeview-controllable (which didn't follow the naming convention)

Changed

  • Revised experimental code documentation
  • Revised feature flag documentation in storybook
  • Update TreeView source to use either of the flags for the controllable API, for backwards compatibility

Removed

  • enable-v11-release flag from storybook docs, it's not used anywhere in the codebase

Testing / Reviewing

  • Treeview feature flag stories should still function as they did before
  • Does this approach make sense to you? Is there anything else we should consider? What might I have forgot or left out? 🤔

This spins off some new work that we'll need to tackle:

@netlify Netlify
Copy link

netlify bot commented Mar 14, 2024

✅ Deploy Preview for v11-carbon-react ready!

Name Link
🔨 Latest commit 6455e51
🔍 Latest deploy log https://app.netlify.com/sites/v11-carbon-react/deploys/65f34952d8c15b0008a5d671
😎 Deploy Preview https://deploy-preview-15964--v11-carbon-react.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@netlify Netlify
Copy link

netlify bot commented Mar 14, 2024 •
edited
Loading

✅ Deploy Preview for v11-carbon-react ready!

Name Link
🔨 Latest commit e566e8e
🔍 Latest deploy log https://app.netlify.com/sites/v11-carbon-react/deploys/66070a5d0390a500083baa55
😎 Deploy Preview https://deploy-preview-15964--v11-carbon-react.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

This was referenced Mar 14, 2024
Open
Open
@tay1orjones tay1orjones marked this pull request as ready for review March 14, 2024 19:45
@tay1orjones tay1orjones requested a review from a team as a code owner March 14, 2024 19:45
@tay1orjones
Copy link
Member Author

@kennylam @janhassel @lee-chase @matthewgallo @elycheea I would love your input here if you have any thoughts

@tay1orjones tay1orjones requested a review from tw15egan March 14, 2024 19:56
janhassel
janhassel reviewed Mar 15, 2024
View reviewed changes
Copy link
Member

@janhassel janhassel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great write-up! Overall I think this makes a lot of sense and is a great way forward. Just left two thoughts.

Additionally, I think it might be good to first tackle and finalize #15966 before this to avoid confusion over that aspect with the tree view flag (consistent deprecation warning, extracting the logic you added to the TreeView component to the useFeatureFlag hook).

@laurenmrice laurenmrice mentioned this pull request Mar 15, 2024
Closed
22 tasks
@tay1orjones
Copy link
Member Author

I think it might be good to first tackle and finalize #15966

Yeah on second thought I think I'm going to rip out the flag work from this PR and have it be part of #15966. enable-treeview-controllable will be perfect for testing the new alias work.

andreancardona
andreancardona approved these changes Mar 26, 2024
View reviewed changes
Copy link
Contributor

@andreancardona andreancardona left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yayyy 😎 looks so good!

@tay1orjones tay1orjones requested a review from janhassel March 29, 2024 18:25
@tay1orjones
Copy link
Member Author

@alisonjoseph @janhassel this is updated and ready for a re-review

janhassel
janhassel approved these changes Apr 2, 2024
View reviewed changes
Copy link
Member

@janhassel janhassel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!

@tay1orjones tay1orjones added this pull request to the merge queue Apr 3, 2024
Merged via the queue into carbon-design-system:main with commit 7d3d800 Apr 3, 2024
20 checks passed
@tay1orjones tay1orjones deleted the feature-flag-naming-convention branch April 3, 2024 16:19
preetibansalui pushed a commit to tay1orjones/carbon that referenced this pull request Apr 24, 2024
@tay1orjones @preetibansalui
docs(featureflags): define naming convention, unify existing flag nam…
538e2cb 
…es (carbon-design-system#15964)

* docs(featureflags): define naming convention, unify existing flag names

* docs(featureflags): rephrase content
@Kritvi-bhatia17 Kritvi-bhatia17 mentioned this pull request Jun 17, 2024
Closed
9 tasks
@Kritvi-bhatia17 Kritvi-bhatia17 mentioned this pull request Jun 19, 2024
Closed
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alisonjoseph alisonjoseph

@tw15egan tw15egan

@janhassel janhassel

@andreancardona andreancardona

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Document feature flag naming convention
5 participants
@tay1orjones @alisonjoseph @tw15egan @janhassel @andreancardona