Merge S1 and S2 chromatic storybooks #6965

LFDanLu · 2024-08-27T23:04:51Z

Closes

✅ Pull Request Checklist:

Included link to corresponding React Spectrum GitHub Issue.
Added/updated unit tests and storybook for this change (for new code or code which already has tests).
Filled out test instructions.
Updated documentation (if it already exists for this component).
Looked at the Accessibility Practices for this feature - Aria Practices

📝 Test Instructions:

🧢 Your Project:

.chromatic/.parcelrc

rspbot · 2024-08-27T23:25:05Z

Build successful! 🎉

rspbot · 2024-08-28T18:40:26Z

Build successful! 🎉

rspbot · 2024-08-28T19:00:07Z

Build successful! 🎉

also make s2 appear last in chromatic storybook

rspbot · 2024-08-30T00:48:57Z

Build successful! 🎉

LFDanLu · 2024-08-30T18:02:21Z

https://www.chromatic.com/build?appId=5f0dd5ad2b5fc10022a2e320&number=774 Standard chromatic
https://www.chromatic.com/build?appId=651b502f6b74abaae4ee734c&number=38 HCM chromatic

take a look at the results, happy for opinions. I've failed several of the stories, will fix/make followup tickets later

LFDanLu · 2024-08-30T18:04:55Z

.chromatic/custom-addons/chromatic/index.js

+// TODO: can't seem to use the style macro here...
+// import {style} from '../../../packages/@react-spectrum/s2/style/spectrum-theme' with {type: 'macro'};


Not exactly sure why the macro doesn't work here, ideally I'd use if for some of the divs below

can't import directly from @react-spectrum/s2 either? instead of the relative path?

yep, doing that returns an error of

@parcel/transformer-js: Error loading macro: Could not resolve module "@react-spectrum/s2/style" from "/Users/danilu/dev/react-spectrum/.chromatic/custom-addons/chromatic/index.js"

The relative path way returns an error of

@parcel/core: Failed to resolve '0' from './.chromatic/custom-addons/chromatic/index.js' @parcel/resolver-default: Cannot find module '0'

I haven't had time to dig into too deeply but its not a huge deal IMO since it is just some padding/div wrappers for story layout

o, maybe this is loaded through another bundler, storybook UI has one vs the one used for stories

.chromatic/custom-addons/chromatic/index.js

rspbot · 2024-08-30T18:18:29Z

Build successful! 🎉

rspbot · 2024-09-05T19:09:43Z

Build successful! 🎉

ktabors · 2024-09-05T21:38:18Z

.chromatic/custom-addons/chromatic/index.js

  }
 });

+function RenderS2({getStory, context, options, selectedLocales, height, minHeight}) {


Could you make this into a shared component that .chromatic-fc and .chromatic both use? I assume there is no way you could make this general enough to make it not specific to s2 or RSPv3, the s2 or RSP3 parts are passed in, or toggles between them based on a property? It's just a big ugly block of code getting repeated. It's probably not worth the effort to refactor it this way.

As part of this, I was wondering if making it a component would move the style macro import out of this file and into another file where it might work.

for now I'll leave it as its own block until I figure out the full set of differences when I add the powerset prop combination stories for s2, but yeah it would be nice to unify this

packages/@react-spectrum/s2/stories/DateField.stories.tsx

packages/@react-spectrum/s2/stories/StyleMacro.stories.tsx

…hromatic

rspbot · 2024-09-09T17:21:44Z

Build successful! 🎉

… crash

LFDanLu · 2024-09-09T18:47:15Z

packages/@react-spectrum/s2/stories/Checkbox.stories.tsx

@@ -17,13 +17,15 @@ import {style} from '../style/spectrum-theme' with {type: 'macro'};
 const meta: Meta<typeof Checkbox> = {
  component: Checkbox,
  parameters: {
-    layout: 'centered'
+    layout: 'centered',
+    chromatic: {delay: 10000}


Note: will remove this delay soon, just added as part of Chromatic support debugging for S2 font vertical centering

rspbot · 2024-09-09T19:01:05Z

Build successful! 🎉

was only needed to try and fix font rendering oddness

rspbot · 2024-09-17T20:30:45Z

Build successful! 🎉

snowystinger · 2024-09-17T23:52:21Z

.chromatic/custom-addons/chromatic/index.js

+  let colorSchemes = options.colorSchemes || S2ColorThemes;
+  let backgrounds = options.backgrounds || ['base'];
+
+  // TODO: there is perhaps some things that can still be shared between the two renders but figured it be easiest to just split them out for the most part


Can remove todo

* add play functions to s2 chromatic overlay stories * move overlay chromatic stories into separate stories this is because we can only have one overay open at a time and so the open overlay doesnt cover the other elements in the main chromatic stories * add RTL and dark theme stories for overlay chromatics stories * add more power set stories and fix lint * fix chromatic error * fix lint and add button + checkbox powersets * add colorarea,field and reusue shortName * Add colorslider, add contextual help, and use original stories in chromatic ones if needed * split up powersets that were too big * ugh lint * adding data-testids and other review comments

rspbot · 2024-09-23T23:09:15Z

Build successful! 🎉

ktabors · 2024-09-23T23:38:23Z

packages/@react-spectrum/s2/chromatic/Breadcrumbs.stories.tsx

+export const Dynamic = Many as StoryObj;
+
+Dynamic.parameters = {
+  // TODO: move these options back to meta above once we get strings for ar-AE. This is just to prevent the RTL story's config from actually applying


Do we have translations since we're about to release?

as far as i know, i don't think we've gotten the translations for breadcrumbs yet

ktabors · 2024-09-23T23:40:17Z

packages/@react-spectrum/s2/chromatic/BreadcrumbsRTL.stories.tsx

  parameters: {
-    layout: 'centered'
-  }
+    chromaticProvider: {colorSchemes: ['dark'], backgrounds: ['base'], locales: ['ar-AE'], disableAnimations: true}


Do these props or the Powerset export's props win? I'm trying to figure out why the colorSchemes and locales don't seem consistent in the Beardcrumb and BreadcrumbRTL stories.

i'm not 100% sure but i think these props win. this comment from daniel might be helpful

ktabors · 2024-09-23T23:48:24Z

packages/@react-spectrum/s2/chromatic/Menu.stories.tsx

+
+export const Default = {...Example};
+
+Default.play = async ({canvasElement}) => {


Do we plan to have submenu stories via "play" too?

Yep, daniel and i have a canvas tracking what things we'd like to add. figured we get this in and i can work on adding additional s2 chromatic stories separately as follow-up

ktabors · 2024-09-23T23:49:45Z

packages/@react-spectrum/s2/chromatic/Tooltip.stories.tsx

+
+export default meta;
+
+export const Default = Example as StoryObj;


I guess we can't do powersets, but can we manually check the tooltip directions?

oh yeah, i think that would be a good idea. i'll add it to the canvas to track

rspbot · 2024-09-24T00:31:45Z

Build successful! 🎉

snowystinger

Notes for follow-up
https://www.chromatic.com/test?appId=5f0dd5ad2b5fc10022a2e320&id=66f207aebe7f86d8a2172645 dialog stories need some fixing, I don't know what I'm looking at here

this story is broken, might need a fix in TagGroup
https://www.chromatic.com/test?appId=5f0dd5ad2b5fc10022a2e320&id=66f207aebe7f86d8a21725e1

snowystinger · 2024-09-24T00:45:27Z

This changed no source files, so merged, we can iterate on it now and catch regressions

roughly got the chromatic stories for S1 and S2 working

7525e66

LFDanLu commented Aug 27, 2024

View reviewed changes

.chromatic/.parcelrc Outdated Show resolved Hide resolved

Add variable font and update titles

838482b

LFDanLu changed the title ~~(WIP) Merge S1 and S2 chromatic storybooks~~ Merge S1 and S2 chromatic storybooks Aug 28, 2024

updating FC storybook too

64731a0

LFDanLu added the in sprint label Aug 28, 2024

LFDanLu added 3 commits August 29, 2024 11:44

move s2 stories into s2 sub category for chromatic merge

391f2d4

also make s2 appear last in chromatic storybook

update chromatic forced colors storybook order too

1df6f3d

use S2 provider for s2 chromatic

72b86e1

remove s2 storys that dont have finished components yet from snapshots

2105607

LFDanLu commented Aug 30, 2024

View reviewed changes

.chromatic/custom-addons/chromatic/index.js Outdated Show resolved Hide resolved

LFDanLu marked this pull request as ready for review August 30, 2024 18:09

reidbarber previously approved these changes Aug 30, 2024

View reviewed changes

Merge branch 'main' into combine_chromatic

1efa254

ktabors reviewed Sep 5, 2024

View reviewed changes

LFDanLu added 3 commits September 6, 2024 11:33

Merge branch 'main' of github.com:adobe/react-spectrum into combine_c…

6c9fb4b

…hromatic

test delay

28ffdae

longer delay

058e425

LFDanLu dismissed reidbarber’s stale review via 058e425 September 9, 2024 16:44

remove non implemented stories, opt into layers, fix breadcrumb story…

1cee66f

… crash

LFDanLu commented Sep 9, 2024

View reviewed changes

remove test delay

99b388e

was only needed to try and fix font rendering oddness

snowystinger previously approved these changes Sep 18, 2024

View reviewed changes

adobe deleted a comment from rspbot Sep 18, 2024

LFDanLu added the release label Sep 18, 2024

yihuiliao dismissed snowystinger’s stale review via 06be29b September 23, 2024 22:50

adobe deleted a comment from rspbot Sep 23, 2024

ktabors approved these changes Sep 23, 2024

View reviewed changes

Merge branch 'main' into combine_chromatic

46fc929

snowystinger approved these changes Sep 24, 2024

View reviewed changes

snowystinger merged commit 5a0b4fa into main Sep 24, 2024
31 checks passed

snowystinger deleted the combine_chromatic branch September 24, 2024 00:45

		// TODO: can't seem to use the style macro here...
		// import {style} from '../../../packages/@react-spectrum/s2/style/spectrum-theme' with {type: 'macro'};


		export const Default = {...Example};

		Default.play = async ({canvasElement}) => {


		export default meta;

		export const Default = Example as StoryObj;

Merge S1 and S2 chromatic storybooks #6965

Merge S1 and S2 chromatic storybooks #6965

Uh oh!

Conversation

LFDanLu commented Aug 27, 2024

✅ Pull Request Checklist:

📝 Test Instructions:

🧢 Your Project:

Uh oh!

Uh oh!

rspbot commented Aug 27, 2024

Uh oh!

rspbot commented Aug 28, 2024

Uh oh!

rspbot commented Aug 28, 2024

Uh oh!

rspbot commented Aug 30, 2024

Uh oh!

LFDanLu commented Aug 30, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rspbot commented Aug 30, 2024

Uh oh!

rspbot commented Sep 5, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

rspbot commented Sep 9, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rspbot commented Sep 9, 2024

Uh oh!

rspbot commented Sep 17, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rspbot commented Sep 23, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yihuiliao Sep 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rspbot commented Sep 24, 2024

Uh oh!

snowystinger left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

snowystinger commented Sep 24, 2024

Uh oh!

Uh oh!

yihuiliao Sep 23, 2024 •

edited

Loading