feat(ui, components): style the loader with the design-system

[alexandre-lelain]

Issue: #9127

What I did

• The Spinner style from @storybook/design-system was applied to the Loader component.

How to test

You can either open the preview deploy and test that the Loader's style is following this one, or checkout the branch, build the project and run the official storybook.

What it looks like

[vercel]

This pull request is being automatically deployed with ZEIT Now (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://zeit.co/storybook/monorepo/26q8yhhyb
✅ Preview: https://monorepo-git-fork-alexandre-lelain-feat-designloader.storybook.now.sh

[domyen]

Thanks @alexandre-lelain, I'll piggy back on this to add some theming support 🙏

[domyen]

I found a regression where the loading spinner shows up in cases that it's not supposed to.

This shouldn't appear in the App:default story.

The App:default, Preview:no tabs, and Preview:with tabs should not render the Loader. This build in Chromatic is where it was originally accepted (by mistake).

The only stories that should be affected by the Loader work are the ones @alexandre-lelain added App:loading state and Loader:infinite state.

Can you take a look at why Loader is being rendering in the other components and update the stories if needed? 🙏

[domyen]

See comment #9249 (comment)

[alexandre-lelain]

The fact the Loader is being displayed on those stories is "expected" - just like the placeholders for the stories' names on the left.

The Loader relies on the same core logic as the stories' names placeholders. It will be displayed automatically when the UI is in loading state (ie after page has loaded, when the stories' JS bundles are being fetched).

Maybe we can override this in the stories, but I'm not sure we can do this without adding some code in the components' sources - which, in my opinion, will add potential source of errors.

[domyen]

Hey Alex, the stories are presentational and used for visual testing. Our goal is to qa the expected experience from the eyes of our users. In this case, we should mock the loader somehow so it doesn’t show up in the other cases. For example, the app:default should not show the loader and app:loading should. Or change how the logic works. Ultimately, we can’t merge this or the other PR without updating the tests in a project of this size.

…

On Sat, Dec 28, 2019 at 10:20 AM Alexandre Le Lain ***@***.***> wrote: The fact the Loader is being displayed on those stories is "expected" - just like the placeholders for the stories' names on the left. The Loader relies on the same core logic as the stories' names placeholders. It will be displayed automatically when the UI is in loading state (ie after page has loaded, when the stories' JS bundles are being fetched). Maybe we can override this in the stories, but I'm not sure we can do this without adding some code in the components' sources - which, in my opinion, will add potential source of errors. — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#9249?email_source=notifications&email_token=AACAJWJGLU7WHG5XRLIUZELQ25VEDA5CNFSM4J7KPZP2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEHYL4LQ#issuecomment-569425454>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACAJWJFJJ5OWXAEXDVCYGLQ25VEDANCNFSM4J7KPZPQ> .

[alexandre-lelain]

Alright, I'll see how I can mock the loader in those cases 👍

[alexandre-lelain]

Hey @domyen I pushed some changes to hide the Loader when needed.

However it seems I cannot update the chromatic snapshots from my machine (the scripts/test.js crashes) - I suspect it's because I'm on Windows (I have nodejs@10.16.3).

But you can see that the loader was hidden correctly in the App:default story.

[domyen]

Thanks alexandre, on holiday right now but will review when I get back to a computer 🙏 🙇

…

On Thu, Jan 2, 2020 at 5:03 AM Alexandre Le Lain ***@***.***> wrote: @alexandre-lelain <https://github.com/alexandre-lelain> requested your review on: #9249 <#9249> feat(ui, components): style the loader with the design-system. — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#9249?email_source=notifications&email_token=AACAJWLEPXOXS4OMGAKX3FTQ3XCZXA5CNFSM4J7KPZP2YY3PNVWWK3TUL52HS4DFWZEXG43VMVCXMZLOORHG65DJMZUWGYLUNFXW5KTDN5WW2ZLOORPWSZGOVXSOPCY#event-2917459851>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACAJWMJAVMYJ7OTQQOPPFDQ3XCZXANCNFSM4J7KPZPQ> .

[ndelangen]

LGTM

It's been corrected