chore(storybook) update storybook format and add ltr/rtl support #12911

millmason · 2022-05-17T18:36:31Z

chore(storybook) add LinkedAccounts to stories

chore(storybook) refactor nav stories

fix(storybook) use addon to get ltr/rtl in storybook

Because

Styles in storybook did not match what we were seeing in prod/local/staging. Also, some cases were missing from demos of the Settings page and nav.

This pull request

Adds in additional cases (specifically, an account with Linked Accounts) and also adds a storybook addon to enable ltr/rtl specific styles

Issue that this pull request solves

Closes: #12848

Checklist

Put an x in the boxes that apply

My commit is GPG signed.
If applicable, I have modified or added tests which pass locally.
I have added necessary documentation (if appropriate).
I have verified that my changes render correctly in RTL (if appropriate).

Screenshots (Optional)

Before (no linked accounts stories):

After (with linked accounts stories):

Before (without rtl/ltr addon):

After (with rtl/ltr addon):

Other information (Optional)

The reason why the styles in storybook looked wrong, even thought they looked fine when rendered outside of storybook, is because some of our styles were only being applied in the case of (under the hood in our CSS), the property selectors [dir='rtl'] or [dir='ltr'] being true. Storybook doesn't have a value for that property available out of the box, so those styles were not being applied. Adding in the storybook-addon-rtl addon, and calling initializeRTL() in preview.js provided that property and fixed the styles. Long-term there is probably future work to be done regularizing some of the styles (so that similar elements have similar styles) and possibly adding a knob so that users can toggle the rtl/ltr in Storybook, but that feels outside of the scope of this ticket.

Testing

Elements where you can see the rtl/ltr fixes in storybook are: the Checkbox story, and the space between the Email Communications label and its associated "link" button, visible in the "basic" story under the Nav story.

fxa-bananafox · 2022-05-17T18:36:46Z

I got u millsoper. 💯

millmason · 2022-05-17T18:54:09Z

packages/fxa-settings/src/components/Nav/index.stories.tsx

+import { Account } from '../../models/Account';
+import { MOCK_LINKED_ACCOUNTS } from '../LinkedAccounts/mocks';
+
+const defaultStory = { title: 'Components/Nav', component: Nav };


Updated format per this comment #12870 (comment)

I think it might be more ideal to format this like:

export default { title: 'Components/Nav', component: Nav, } as Meta; // or "Basic" as you have it further below, I personally prefer "Default" since it's slightly more clear (my opinion) export const Default = () => <Nav />;

This is more similar to how they recommend it in the docs and how we've done all of our other modern default story SB formatting in the few fxa-settings files as well as our email stories.

LZoog

Thank you so much for fixing this 🙌

I just had some nits/suggestions, but then a couple of notes as I've dug a little here.

I noticed in the package description, it states:

A storybook addons that lets your users toggle between ltr and rtl.

But I'm not sure I see the toggle shown there, and I know you noted in your description:

Long-term there is probably future work to be done regularizing some of the styles (so that similar elements have similar styles) and possibly adding a knob so that users can toggle the rtl/ltr in Storybook, but that feels outside of the scope of this ticket.

... I actually forgot that we added a toggle for this in fxa-auth-server storybook:

I don't think this throws a wrench into your PR (as in, I don't believe you need to uninstall this package and reference auth-server's SB), since I don't think we have any actual RTL specific styles in our emails since they're all centered and pretty basic. I didn't dig further into how that package can toggle vs what we're doing, but I wanted to note that just for consistency's sake whether you do something here with it or we do so in the future.

I noticed we actually have a duplicate ticket for this - #9195 - I didn't realize when I filed the ticket your PR fixes that this was an RTL/LTR problem, and forgot about that ticket until now. We could cancel it in favor of the ticket this closes, but since that ticket actually references a toggle, we could instead slightly modify the title and leave a comment over there saying the style problem was fixed but we'd still like to have an option to switch from RTL/LTR, with maybe a reference to this comment or with just a summary of it so we can make sure whatever we do we're consistent.

LZoog · 2022-05-18T22:31:13Z

packages/fxa-settings/src/components/Nav/index.stories.tsx

+import { Account } from '../../models/Account';
+import { MOCK_LINKED_ACCOUNTS } from '../LinkedAccounts/mocks';
+
+const defaultStory = { title: 'Components/Nav', component: Nav };


I think it might be more ideal to format this like:

export default { title: 'Components/Nav', component: Nav, } as Meta; // or "Basic" as you have it further below, I personally prefer "Default" since it's slightly more clear (my opinion) export const Default = () => <Nav />;

This is more similar to how they recommend it in the docs and how we've done all of our other modern default story SB formatting in the few fxa-settings files as well as our email stories.

LZoog · 2022-05-18T22:44:44Z

packages/fxa-settings/src/components/Nav/index.stories.tsx

+const accountWithLinkedAccounts = Object.assign({}, account, {
+  linkedAccounts: MOCK_LINKED_ACCOUNTS,
+});
+
+const configWithoutNewsletterLink = Object.assign({}, getDefault(), {
+  marketingEmailPreferencesUrl: '',
+});


It's fine to leave these as-is since this is just an opinion but I personally prefer this syntax since it reads more clear to me and we're doing similar stuff in emails Storybooks:

const accountWithLinkedAccounts = { ...account, linkedAccounts: MOCK_LINKED_ACCOUNTS, }); const configWithoutNewsletterLink = { ...getDefault(), marketingEmailPreferencesUrl: '', });

Do you prefer explicitly using Object.assign?

I don't, this is way clearer to me also! 👍

LZoog · 2022-05-18T22:45:41Z

packages/fxa-settings/src/components/Nav/index.stories.tsx


 const account = {
  primaryEmail: {
    email: 'johndope@example.com',
  },
  subscriptions: [{ created: 1, productName: 'x' }],
+  linkedAccounts: [],
 } as any;


Slightly better typing here than any is as unknown as Account.

That's really cool thank you!!

LZoog · 2022-05-18T22:47:44Z

packages/fxa-settings/src/components/Nav/index.stories.tsx

+  marketingEmailPreferencesUrl: '',
+});
+
+const storyWithAccount = (


I named a similar wrapper in Security this (storyWithAccount) since account was the only param needed. This might be better named to something like storyWithContext since that's what the story is providing, but I'm open to other name suggestions and it's not a big deal to leave it as-is.

Also, I think you can use the type Config for config.

Excellent, great idea!

LZoog · 2022-05-18T23:16:19Z

packages/fxa-settings/src/components/PageSettings/index.stories.tsx

@@ -26,6 +27,7 @@ storiesOf('Pages/Settings', module)
          recoveryKey: false,
          totp: { exists: false, verified: false },
          attachedClients: SERVICES_NON_MOBILE,
+          linkedAccounts: MOCK_LINKED_ACCOUNTS,


Not necessary for this PR but it would also be super nice for this file to get reformatted to the new Storybook since it was touched 🙈

I think maybe for "Cold start" story (this one), we might want to not show linkedAccounts. That story is supposed to be a "bare minimum" example if someone just created an account - I think in fact it would be nice for attachedClients to just show one session, but that again isn't part of the issue you fixed, so 🤷‍♀️

Great thinking, will do!

fxa-bananafox · 2022-05-31T23:11:18Z

I got u millsoper. 💯

fxa-bananafox · 2022-06-01T18:41:14Z

I got u millsoper. 💯

LZoog

LGTM! Let's be sure to address this comment as I know you said you were going to in Slack 🙂 Also, don't forget to fixup your 2nd commit too and tweak the commit message to follow that because/this commit format.

I'm also fine with us closing that ticket (#9195) saying it's fixed with this PR and you could file a new one with context from this PR, the ticket it closes, and #9195 noting the consistency bit between our SBs. That gives us the advantage of triaging it in our next meeting, but again I'm also personally fine with just tweaking the title and leaving a comment over there since it was part of the issue anyway and I know we'll just backlog it.

LZoog · 2022-06-01T20:45:56Z

packages/fxa-settings/src/components/Nav/index.stories.tsx

+  return story;
+};
+
+export const basic = () => <Nav />;


Nit: this should be capitalized conventionally, e.g. Basic

LZoog · 2022-06-01T20:47:21Z

packages/fxa-settings/src/components/PageSettings/index.stories.tsx

+  component: PageSettings,
+} as Meta;
+
+const coldStartAccount = {


Yaaaaas thank you for the refactor 😍

LZoog · 2022-06-01T20:48:51Z

packages/fxa-settings/src/components/PageSettings/index.stories.tsx

+  linkedAccounts: MOCK_LINKED_ACCOUNTS,
+}
+
+const storyWithContext = (


Just thinking outloud; if we end up using storyWithContext in our other SB refactors, we should definitely consider pulling this out into a helper function to use across stories, we can have it take a children prop.

Mm I like that idea. It would feel nice to do a big sweep refactoring all of these and refactor it out then

LZoog · 2022-06-01T20:49:45Z

packages/fxa-settings/src/components/PageSettings/index.stories.tsx

+
+export const ColdStart = storyWithContext(
+  coldStartAccount,
+  'cold start',


I don't mind either way, but you could take out the story names here. They'll just show up in SB as Cold Start vs cold start. 🤷‍♀️

oh interesting hmmmmm. I liked seeing them in the example I was modeling this on just because it was a little more explicit/less mysterious, but I think that confusion will become less and less frequent the more of these that we transition. Good with going either way!

pdehaan · 2022-06-01T23:15:28Z

Unrelated, but... Should we add this to all the Storybook projects?

Because: * RTL/LTR styles weren't showing up in storybook. This commit: * Uses an addon to add the `dir` value so that rtl/ltr styles are applied * Adds in Linked Accounts to some of the stories * Update the format for files that were changed Closes: #12848

millmason commented May 17, 2022

View reviewed changes

millmason marked this pull request as ready for review May 17, 2022 18:54

millmason requested a review from a team as a code owner May 17, 2022 18:54

millmason changed the title ~~[FXA-5072] Update storybook format and add ltr/rtl support~~ chore(storybook) update storybook format and add ltr/rtl support May 17, 2022

LZoog self-assigned this May 17, 2022

LZoog reviewed May 18, 2022

View reviewed changes

millmason force-pushed the FXA-5072-storybook-fixes branch from d734ccf to 811c520 Compare May 31, 2022 23:11

millmason force-pushed the FXA-5072-storybook-fixes branch from eecf753 to ca1616e Compare June 1, 2022 18:40

LZoog approved these changes Jun 1, 2022

View reviewed changes

millmason mentioned this pull request Jun 2, 2022

Storybook does not use LTR or RTL-specific styles #9195

Closed

millmason force-pushed the FXA-5072-storybook-fixes branch from b08224d to 6b92c6a Compare June 2, 2022 20:24

millmason merged commit 285885e into main Jun 2, 2022

millmason deleted the FXA-5072-storybook-fixes branch July 12, 2022 20:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(storybook) update storybook format and add ltr/rtl support #12911

chore(storybook) update storybook format and add ltr/rtl support #12911

millmason commented May 17, 2022 •

edited

Loading

fxa-bananafox bot commented May 17, 2022

millmason May 17, 2022

LZoog May 18, 2022

LZoog left a comment

LZoog May 18, 2022

LZoog May 18, 2022

millmason May 31, 2022

LZoog May 18, 2022

millmason May 31, 2022

LZoog May 18, 2022 •

edited

Loading

millmason May 31, 2022

LZoog May 18, 2022

millmason May 31, 2022

fxa-bananafox bot commented May 31, 2022

fxa-bananafox bot commented Jun 1, 2022

LZoog left a comment •

edited

Loading

LZoog Jun 1, 2022

LZoog Jun 1, 2022

LZoog Jun 1, 2022

millmason Jun 2, 2022

LZoog Jun 1, 2022

millmason Jun 2, 2022

pdehaan commented Jun 1, 2022

chore(storybook) update storybook format and add ltr/rtl support #12911

chore(storybook) update storybook format and add ltr/rtl support #12911

Conversation

millmason commented May 17, 2022 • edited Loading

Because

This pull request

Issue that this pull request solves

Checklist

Screenshots (Optional)

Other information (Optional)

Testing

fxa-bananafox bot commented May 17, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LZoog left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LZoog May 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fxa-bananafox bot commented May 31, 2022

fxa-bananafox bot commented Jun 1, 2022

LZoog left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pdehaan commented Jun 1, 2022

millmason commented May 17, 2022 •

edited

Loading

LZoog May 18, 2022 •

edited

Loading

LZoog left a comment •

edited

Loading