Skip to content

fix: Fix storybook blockers - #443

Merged
bryphe-coder merged 1 commit into
mainfrom
bryphe/fix/storybook-build
Mar 15, 2022
Merged

fix: Fix storybook blockers#443
bryphe-coder merged 1 commit into
mainfrom
bryphe/fix/storybook-build

Conversation

@bryphe-coder

@bryphe-coder bryphe-coder commented Mar 15, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

bryphe-coder commented Mar 15, 2022

Copy link
Copy Markdown
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

greyscaled commented Mar 15, 2022

Copy link
Copy Markdown
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

@bryphe-coder bryphe-coder mentioned this pull request Mar 15, 2022
Closed
@codecov

codecov Bot commented Mar 15, 2022 •
edited
Loading

Copy link
Copy Markdown

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

bryphe-coder commented Mar 15, 2022

Copy link
Copy Markdown
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

Reviewers

@kylecarbs kylecarbs kylecarbs approved these changes
+1 more reviewer
@greyscaled greyscaled greyscaled approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@bryphe-coder @greyscaled @kylecarbs