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.

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

[WNMGDS-2439] Replace many config functions with single global config function #2889

Merged
merged 13 commits into from
Jan 18, 2024

Conversation

pwolfert
Copy link
Contributor

Summary

WNMGDS-2439

Introduces the new JavaScript config interface defined in our RFC alongside our old way of setting feature flags, which are now deprecated.

From the RFC:

The design system uses a handful of "flags" to configure various aspects of our JavaScript library. For instance, analytics can be turned on for all Alert components by passing true to setAlertSendsAnalytics. Right now the HealthCare.gov Design System is the only one left that needs to set custom configs that apply to all applications in its domain, and that's the default error placement configuration. All the other configuration settings are managed by individual applications using the design system. I propose that the one remaining setting be made the responsibility of the applications, but we can also make some changes to improve the developer experience.

  • One config function to rule them all (one function to bind them...)
  • Adds deprecation warnings to the individual feature flag functions
  • Healthcare package currently still has the side effect of calling the config function itself. This will only be changed if we consolidate the packages.
  • Creates a new Global config page that explains how the new config function works. I made it live under "components" because it only really applies to our component library. This may be a controversial decision.
  • Creates a new Component analytics page in the same "components" directory that explains how to configure and use analytics, moving it out of the getting-started documentation for developers

How to test

  1. Instructions on how to test the changes in this PR.

Checklist

  • Prefixed the PR title with the Jira ticket number as [WNMGDS-####] Title or [NO-TICKET] if this is unticketed work.
  • Selected appropriate Type (only one) label for this PR, if it is a breaking change, label should only be Type: Breaking
  • Selected appropriate Impacts, multiple can be selected.
  • Selected appropriate release milestone

If this is a change to code:

  • Created or updated unit tests to cover any new or modified code
  • If necessary, updated unit-test snapshots (yarn test:unit:update) and browser-test snapshots (yarn test:browser:update)

If this is a change to documentation:

  • Checked for spelling and grammatical errors

Introduce the new JavaScript config interface [defined in our RFC](#2435) alongside our old way of setting feature flags
I'm thinking I might have another analytics setting to add to the config, and then maybe the analytics docs can all live in one place
And fix an inaccuracy in one of the doc comments
@pwolfert pwolfert added Impacts: Core Impacts the core DS primarily, changes may occur in other themes as well. Type: Added Indicates a new feature. labels Jan 16, 2024
@pwolfert pwolfert added this to the 9.0.0-beta.3 milestone Jan 16, 2024
@pwolfert pwolfert requested a review from zarahzachz January 16, 2024 22:15
// Deprecated stuff

function depWarning(fnName: string) {
if (process.env.NODE_ENV !== 'production') {
Copy link
Collaborator

Choose a reason for hiding this comment

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

i wonder if these errors are best seen when the env is dev? do we want to litter the console in prod with these warnings?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's targetting non-production. Did you find it working otherwise through testing?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think I misread the code. Disregard!

@pwolfert pwolfert merged commit 6ef8f37 into main Jan 18, 2024
1 check passed
@pwolfert pwolfert deleted the pwolfert/new-config branch January 18, 2024 00:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Impacts: Core Impacts the core DS primarily, changes may occur in other themes as well. Type: Added Indicates a new feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants