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

Docs: added cookies to support activation/deactivation of experiments from ExperimentProvider in Docs #2511

Merged

Conversation

AlbertCarreras
Copy link
Contributor

@AlbertCarreras AlbertCarreras commented Nov 18, 2022

Summary

What changed?

Docs: added cookies to support activation/deactivation of experiments from ExperimentProvidern in Docs

Why?

Be able to support SlimBanner's primaryAction to toggle between active/inactive experiments to visualize changes from experiments.

Implemented in Textfield. See https://deploy-preview-2511--gestalt.netlify.app/web/textfield

Next ones:
Screen Shot 2022-11-18 at 1 12 00 PM
Screen Shot 2022-11-18 at 1 56 21 PM

Links

  • Jira
  • [TDD](link to Paper doc)
  • [Figma](link to Figma file)

Checklist

  • Added unit and Flow Tests
  • Added documentation + accessibility tests
  • Verified accessibility: keyboard & screen reader interaction
  • Checked dark mode, responsiveness, and right-to-left support
  • Checked stakeholder feedback (e.g. Gestalt designers)

@AlbertCarreras AlbertCarreras added the patch release Patch release label Nov 18, 2022
@AlbertCarreras AlbertCarreras marked this pull request as ready for review November 18, 2022 18:23
@AlbertCarreras AlbertCarreras requested a review from a team as a code owner November 18, 2022 18:23
@netlify
Copy link

netlify bot commented Nov 18, 2022

Deploy Preview for gestalt ready!

Name Link
🔨 Latest commit 44a62e9
🔍 Latest deploy log https://app.netlify.com/sites/gestalt/deploys/637bd1f0d379ad0008b513b1
😎 Deploy Preview https://deploy-preview-2511--gestalt.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 settings.

@AlbertCarreras AlbertCarreras force-pushed the experimentsActivator branch 7 times, most recently from b1ff233 to c6be74a Compare November 18, 2022 20:54
@@ -6,7 +6,6 @@ import docgen, { type DocGen } from '../../docs-components/docgen.js';
import PageHeader from '../../docs-components/PageHeader.js';
import MainSection from '../../docs-components/MainSection.js';
import QualityChecklist from '../../docs-components/QualityChecklist.js';

Copy link
Contributor

Choose a reason for hiding this comment

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

unrelated change, should be reverted for a cleaner PR

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 not unrelated, I'm not gonna put a single PR to remove this line.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, it's cleanup that can be addressed the next time someone does work on this file. As it stands, anyone looking into the git history of this file will see this PR, adding noise to their search as this PR has no material changes to the file.

: 'Activate experiments',
onClick: () =>
setExperiments(
experiments === generatedDocGen?.displayName ? '' : generatedDocGen?.displayName,
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like this is all-or-nothing, yes? We should provide users more granularity here. Users should be able to toggle individual experiments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried stringifying an array of experiments but it couldn't get it to work. It's not like we have >1 experiments at the time for each single component. This is a first iteration of this implementation. With the cmp name I hit both desktop and web.

If there's need for refactoring this to allow overlapping experiments we can rethink the implementation in a future PR.,

Copy link
Contributor

Choose a reason for hiding this comment

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

What didn't work about stringifying an array or object of experiments?

iconAccessibilityLabel="Component under experiment"
message={
experiments === generatedDocGen?.displayName
? 'The current Textfield example is displaying experimental changes.'
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: consider consolidating this text by using a template literal and moving the logic to where it's needed, e.g.

message={`The current TextField example is ${experiments === generatedDocGen?.displayName ? '' : 'NOT '}displaying experimental changes.`}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

DONE

@AlbertCarreras AlbertCarreras merged commit 8fb214c into pinterest:master Nov 21, 2022
@dangerismycat dangerismycat changed the title Docs: added cookies to support activation/deactivation of experiments from ExperimentProvidern in Docs Docs: added cookies to support activation/deactivation of experiments from ExperimentProvider in Docs Dec 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
patch release Patch release
Projects
None yet
2 participants