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

fix: Fix storybook blockers #443

Merged
merged 1 commit into from
Mar 15, 2022
Merged

Conversation

bryphe-coder
Copy link
Contributor

@bryphe-coder bryphe-coder commented Mar 15, 2022

I was just testing storybook out on main, and saw some of the components had errors:

image

There were two problems:

  • The <Workspace /> component had new arguments, that don't seem to be validated by yarn storybook:build
  • The top-level react-router-dom is not available in storybook.

After fixing these, all the storybook components work:

image

@bryphe-coder bryphe-coder self-assigned this Mar 15, 2022
@bryphe-coder
Copy link
Contributor Author

Unfortunately the yarn storybook:build command doesn't seem to catch these errors - I guess it's an argument for bringing back chromatic!

@greyscaled
Copy link
Contributor

Unfortunately the yarn storybook:build command doesn't seem to catch these errors - I guess it's an argument for bringing back chromatic!

I would love for us to have Chromatic (Snapshot testing). The only reason it was removed temporarily was because we hadn't set up a proper plan and figured out the best pipeline/usage for it yet.

I do see it as above "nice-to-have" in terms of priority for regression prevention. While visual regressions can seem lower risk, they're still a risk that could disrupt somebodies workflow

@codecov
Copy link

codecov bot commented Mar 15, 2022

Codecov Report

Merging #443 (54e2cc8) into main (97399f8) will increase coverage by 0.10%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main     #443      +/-   ##
==========================================
+ Coverage   68.14%   68.24%   +0.10%     
==========================================
  Files         159      158       -1     
  Lines        9226     9224       -2     
  Branches       73       73              
==========================================
+ Hits         6287     6295       +8     
+ Misses       2317     2310       -7     
+ Partials      622      619       -3     
Flag Coverage Δ
unittest-go-macos-latest 62.82% <ø> (+0.15%) ⬆️
unittest-go-ubuntu-latest 67.43% <ø> (-0.03%) ⬇️
unittest-go-windows-2022 62.22% <ø> (ø)
unittest-js 64.23% <ø> (+0.16%) ⬆️
Impacted Files Coverage Δ
peer/conn.go 80.81% <0.00%> (-0.77%) ⬇️
site/.storybook/preview.js
coderd/provisionerdaemons.go 57.60% <0.00%> (+0.54%) ⬆️
peerbroker/listen.go 83.89% <0.00%> (+2.54%) ⬆️
codersdk/provisionerdaemons.go 61.53% <0.00%> (+3.07%) ⬆️
provisionersdk/serve.go 43.24% <0.00%> (+8.10%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 97399f8...54e2cc8. Read the comment docs.

@bryphe-coder
Copy link
Contributor Author

Agree 100% with your comments @vapurrmaid - I logged #444 to track investigate bringing Chromatic back.

@bryphe-coder bryphe-coder merged commit dfc353b into main Mar 15, 2022
@bryphe-coder bryphe-coder deleted the bryphe/fix/storybook-build branch March 15, 2022 18:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants