Skip to content

Fix Storybook locally#13916

Merged
grgia merged 1 commit intomainfrom
marcaaron-fixStorybook
Feb 9, 2023
Merged

Fix Storybook locally#13916
grgia merged 1 commit intomainfrom
marcaaron-fixStorybook

Conversation

@marcaaron
Copy link
Contributor

@marcaaron marcaaron commented Dec 31, 2022 •
edited
Loading

Details

Noticed that storybook is not working locally anymore because of some url() resolving stuff being now broken.

I think maybe it was from this PR but not too sure cc @Luke9389

Maybe @roryabraham knows a way to avoid the duplication. I looked through the webpack docs and couldn't find a clear answer for how to disable the url() resolving behavior or rewrite the file paths. It always tries to resolve the fonts relative to where the fonts.css file is imported from 🥲

Fixed Issues

$ #14905

Tests

  1. npm i && npm run storybook
  2. Verify the build is not broken anymore.
  • Verify that no errors appear in the JS console

Offline tests

N/A

QA Steps

  1. Visit staging.new.expensify.com/docs/index.html
  2. Verify the storybook site looks normal
  • Verify that no errors appear in the JS console

PR Author Checklist

  • I linked the correct issue in the ### Fixed Issues section above
  • I wrote clear testing steps that cover the changes made in this PR
    • I added steps for local testing in the Tests section
    • I added steps for the expected offline behavior in the Offline steps section
    • I added steps for Staging and/or Production testing in the QA steps section
    • I added steps to cover failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
    • I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
    • I tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).
  • I included screenshots or videos for tests on all platforms
  • I ran the tests on all platforms & verified they passed on:
    • Android / native
    • Android / Chrome
    • iOS / native
    • iOS / Safari
    • MacOS / Chrome / Safari
    • MacOS / Desktop
  • I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
  • I followed proper code patterns (see Reviewing the code)
    • I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick)
    • I verified that comments were added to code that is not self explanatory
    • I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
    • I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
      • If any non-english text was added/modified, I verified the translation was requested/reviewed in #expensify-open-source and it was approved by an internal Expensify engineer. Link to Slack message:
    • I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
    • I verified any copy / text that was added to the app is correct English and approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
    • I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
    • I verified the JSDocs style guidelines (in STYLE.md) were followed
  • If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
  • I followed the guidelines as stated in the Review Guidelines
  • I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
  • I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
  • I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
  • I verified that if a function's arguments changed that all usages have also been updated correctly
  • If a new component is created I verified that:
    • A similar component doesn't exist in the codebase
    • All props are defined accurately and each prop has a /** comment above it */
    • The file is named correctly
    • The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
    • The only data being stored in the state is data necessary for rendering and nothing else
    • For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
    • Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
    • All JSX used for rendering exists in the render method
    • The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
  • If any new file was added I verified that:
    • The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
  • If a new CSS style is added I verified that:
    • A similar style doesn't already exist
    • The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
  • If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
  • If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
  • If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
  • I have checked off every checkbox in the PR author checklist, including those that don't apply to this PR.

Screenshots/Videos

Web 2023-02-07_05-29-06
Mobile Web - Chrome

N/A

Mobile Web - Safari

N/A

Desktop

N/A

iOS

N/A

Android

N/A

@marcaaron marcaaron self-assigned this Dec 31, 2022
@Luke9389
Copy link
Contributor

Luke9389 commented Jan 4, 2023 •
edited
Loading

Interesting, this was running for me locally just fine when testing that PR. Let me try again to see if it's broken for me.

Edit: yep, it's definitely broken for me too.

@marcaaron
Copy link
Contributor Author

marcaaron commented Jan 4, 2023

npm run storybook is specifically borked for me locally. I noticed the staging build is not totally borked but the fonts are off.

2023-01-03_20-17-49

@Luke9389 Luke9389 mentioned this pull request Jan 17, 2023
Closed
1 task
@marcaaron marcaaron changed the title [WIP] Fix Storybook locally Fix Storybook locally Feb 7, 2023
@marcaaron marcaaron marked this pull request as ready for review February 7, 2023 15:40
@marcaaron marcaaron requested a review from a team as a code owner February 7, 2023 15:40
@melvin-bot melvin-bot bot requested review from grgia and mananjadhav and removed request for a team February 7, 2023 15:41
@MelvinBot
Copy link
Contributor

MelvinBot commented Feb 7, 2023 •
edited by marcaaron
Loading

@grgia One of you needs to copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

@marcaaron marcaaron removed the request for review from mananjadhav February 7, 2023 15:41
@marcaaron
Copy link
Contributor Author

marcaaron commented Feb 7, 2023

Removing @mananjadhav we do not need a C+ review here as the testing is primarily on web. Thanks.

grgia
grgia approved these changes Feb 9, 2023
View reviewed changes
Copy link
Contributor

@grgia grgia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests well

@grgia
Copy link
Contributor

grgia commented Feb 9, 2023 •
edited
Loading

Reviewer Checklist

  • I have verified the author checklist is complete (all boxes are checked off).
  • I verified the correct issue is linked in the ### Fixed Issues section above
  • I verified testing steps are clear and they cover the changes made in this PR
    • I verified the steps for local testing are in the Tests section
    • I verified the steps for Staging and/or Production testing are in the QA steps section
    • I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
    • I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
  • I checked that screenshots or videos are included for tests on all platforms
  • I included screenshots or videos for tests on all platforms
  • I verified tests pass on all platforms & I tested again on:
    • Android / native
    • Android / Chrome
    • iOS / native
    • iOS / Safari
    • MacOS / Chrome / Safari
    • MacOS / Desktop
  • If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
  • I verified proper code patterns were followed (see Reviewing the code)
    • I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
    • I verified that comments were added to code that is not self explanatory
    • I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
    • I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
    • I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
    • I verified any copy / text that was added to the app is correct English and approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
    • I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
    • I verified the JSDocs style guidelines (in STYLE.md) were followed
  • If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
  • I verified that this PR follows the guidelines as stated in the Review Guidelines
  • I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
  • I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
  • I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
  • If a new component is created I verified that:
    • A similar component doesn't exist in the codebase
    • All props are defined accurately and each prop has a /** comment above it */
    • The file is named correctly
    • The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
    • The only data being stored in the state is data necessary for rendering and nothing else
    • For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
    • Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
    • All JSX used for rendering exists in the render method
    • The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
  • If any new file was added I verified that:
    • The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
  • If a new CSS style is added I verified that:
    • A similar style doesn't already exist
    • The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
  • If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
  • If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
  • If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
  • I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Web image
Mobile Web - Chrome

N/A

Mobile Web - Safari

N/A

Desktop

N/A

iOS

N/A

Android

N/A

@grgia grgia merged commit 7e6a776 into main Feb 9, 2023
@grgia grgia deleted the marcaaron-fixStorybook branch February 9, 2023 17:21
@OSBotify
Copy link
Contributor

OSBotify commented Feb 9, 2023

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

@github-actions
Copy link
Contributor

github-actions bot commented Feb 9, 2023

Performance Comparison Report 📊

Significant Changes To Duration

There are no entries

Meaningless Changes To Duration

Show entries
Name Duration
App start TTI 688.787 ms → 707.541 ms (+18.754 ms, +2.7%)
App start nativeLaunch 18.938 ms → 20.774 ms (+1.837 ms, +9.7%)
App start runJsBundle 187.125 ms → 187.452 ms (+0.327 ms, ±0.0%)
App start regularAppStart 0.014 ms → 0.015 ms (+0.001 ms, +5.9%)
Open Search Page TTI 608.574 ms → 602.030 ms (-6.544 ms, -1.1%)
Show details
Name Duration
App start TTI Baseline
Mean: 688.787 ms
Stdev: 25.843 ms (3.8%)
Runs: 641.2165430001915 642.5322549999692 651.7830099998973 656.1585679999553 660.11608099984 660.4071470000781 662.4723800001666 664.2936019999906 666.1601530001499 673.260819000192 673.7209930000827 677.0502849998884 678.2311539999209 685.4215879999101 690.3971569999121 690.9179719998501 691.4041140000336 693.2803440000862 693.8902179999277 698.0803100001067 698.6027150000446 702.1858080001548 705.6965580000542 710.0656480002217 711.165963999927 713.7382310000248 714.4455630001612 715.4140159999952 727.8467879998498 729.5373309999704 730.4556599999778 731.2231399999

Current
Mean: 707.541 ms
Stdev: 21.663 ms (3.1%)
Runs: 672.4725410002284 672.560262999963 673.5802589999512 680.7154939998873 684.419916999992 684.5364810000174 689.3867399999872 690.9537080000155 691.3107480001636 694.6677819998004 696.8405229998752 697.0898770000786 700.9023139998317 703.4991779997945 705.5124860000797 707.7441449998878 708.8534200000577 714.3483369997703 715.5582280000672 716.3771359999664 719.8940920000896 720.5956460000016 724.4223259999417 724.8687359998003 731.5430649998598 733.2800199999474 736.095736999996 738.7412069998682 745.3107870002277 750.134440000169
App start nativeLaunch Baseline
Mean: 18.938 ms
Stdev: 1.456 ms (7.7%)
Runs: 17 17 18 18 18 18 18 18 18 18 18 18 18 18 18 18 18 18 19 19 19 19 20 20 20 20 20 20 21 22 22 23

Current
Mean: 20.774 ms
Stdev: 2.536 ms (12.2%)
Runs: 18 18 18 18 18 18 18 18 19 19 19 20 20 20 20 20 21 21 21 21 21 22 22 22 22 24 24 24 24 27 27
App start runJsBundle Baseline
Mean: 187.125 ms
Stdev: 17.501 ms (9.4%)
Runs: 155 162 164 165 167 171 173 174 176 178 179 181 182 182 182 183 184 186 187 191 193 195 196 197 201 203 204 205 207 210 219 236

Current
Mean: 187.452 ms
Stdev: 14.600 ms (7.8%)
Runs: 165 167 170 173 173 174 176 176 178 179 179 180 183 183 183 183 185 186 187 190 194 195 196 197 198 200 201 202 209 224 225
App start regularAppStart Baseline
Mean: 0.014 ms
Stdev: 0.001 ms (4.7%)
Runs: 0.012817000038921833 0.012980000115931034 0.013265000190585852 0.013346000108867884 0.013387000188231468 0.013712000101804733 0.01375299971550703 0.013956000097095966 0.01399700017645955 0.014037999790161848 0.01411899970844388 0.014241999946534634 0.014283000025898218 0.01436399994418025 0.014403999783098698 0.014404000248759985 0.014444999862462282 0.01448499970138073 0.014485999941825867 0.014566999860107899 0.014567000325769186 0.014607999939471483 0.014648000244051218 0.014690000098198652 0.014770000241696835 0.0148930000141263 0.015015000011771917 0.015300000086426735 0.015339999925345182 0.015705999918282032

Current
Mean: 0.015 ms
Stdev: 0.001 ms (4.5%)
Runs: 0.013955999631434679 0.0139979999512434 0.014078999869525433 0.014120000414550304 0.014201000332832336 0.014322999864816666 0.014566999860107899 0.014607999939471483 0.014688999857753515 0.014688999857753515 0.014810999855399132 0.014810999855399132 0.014973999932408333 0.015015000011771917 0.015055000316351652 0.01525900000706315 0.015299000311642885 0.015339999925345182 0.015461999922990799 0.015462000388652086 0.015544000081717968 0.015583999920636415 0.015666000079363585 0.015705999918282032 0.0157880000770092 0.015868999995291233 0.015868999995291233 0.015868999995291233 0.01599099999293685 0.01615400006994605 0.016439000144600868
Open Search Page TTI Baseline
Mean: 608.574 ms
Stdev: 17.916 ms (2.9%)
Runs: 578.6856290004216 581.6145840003155 588.5377200003713 588.6829429999925 590.2828779998235 591.2206220002845 594.8924970002845 595.0942389997654 596.3786220001057 597.808796999976 599.5089520001784 600.7685949997976 601.2583829998039 602.2835699999705 603.2797449999489 605.9476320003159 610.0157469999976 610.4110510000028 611.2545980000868 612.6861580000259 613.0578609998338 615.8168540000916 615.8616540003568 620.2210699999705 623.4922289997339 626.2987060002051 627.1680499999784 631.3762619998306 634.3726809998043 634.5509440000169 662.9720459999517

Current
Mean: 602.030 ms
Stdev: 13.452 ms (2.2%)
Runs: 566.2185469996184 578.6418859995902 583.3868410000578 586.6177570000291 588.7242029998451 588.8856210000813 589.2232260005549 592.3692220002413 593.8186450004578 594.2012940002605 596.2028399999253 598.0559490006417 598.224813000299 598.9370530000888 599.3588870000094 599.7084149997681 599.7150069996715 602.7816980006173 604.5109049999155 607.3957110000774 609.3789069997147 610.5654710000381 610.7604980003089 611.2160240001976 611.3345539998263 612.8363040005788 613.2568770004436 614.1348069999367 615.0270589999855 618.8068030001596 623.4147950001061 624.2937009995803 624.9915780005977

@OSBotify OSBotify mentioned this pull request Feb 10, 2023
Closed
9 tasks
@OSBotify
Copy link
Contributor

OSBotify commented Feb 10, 2023

🚀 Deployed to staging by https://github.com/grgia in version: 1.2.69-0 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@kbecciv kbecciv mentioned this pull request Feb 10, 2023
Closed
6 tasks
@MelvinBot MelvinBot mentioned this pull request Feb 11, 2023
Closed
6 tasks
@OSBotify
Copy link
Contributor

OSBotify commented Feb 11, 2023

🚀 Deployed to production by https://github.com/mountiny in version: 1.2.69-2 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

Luke9389
Luke9389 reviewed Feb 13, 2023
View reviewed changes
.storybook/main.js
Comment on lines 11 to 15
staticDirs: [
'./public',
'../assets/css',
{from: '../assets/css', to: 'css'},
{from: '../assets/fonts/web', to: 'fonts'},
],
Copy link
Contributor

@Luke9389 Luke9389 Feb 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is super cool. Thx @marcaaron 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@grgia grgia grgia approved these changes

+1 more reviewer

@Luke9389 Luke9389 Luke9389 left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@marcaaron marcaaron

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@marcaaron @Luke9389 @MelvinBot @grgia @OSBotify