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

Add storybook and a Header story #1257

Merged
merged 15 commits into from
May 14, 2021
Merged

Add storybook and a Header story #1257

merged 15 commits into from
May 14, 2021

Conversation

marcaaron
Copy link
Contributor

@marcaaron marcaaron commented Jan 16, 2021

Details

Adds Storybook to E.cash

Fixed Issues

Fixes https://github.com/Expensify/Expensify/issues/164021

Tests

  1. npm i
  2. Run npm run storybook
  3. Verify that storybook opens

QA

  1. Navigate to https://staging.expensify.cash/docs
  2. Verify that storybook is accessible

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

2021-05-13_14-04-52

@marcaaron marcaaron self-assigned this Jan 16, 2021
@github-actions
Copy link
Contributor

github-actions bot commented Jan 16, 2021

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@botify
Copy link

botify commented May 13, 2021

Hey, I noticed you changed some webpack configuration files. This can break production builds. Did you remember to run a production build locally to verify they still work?

@botify
Copy link

botify commented May 14, 2021

Hey, I noticed you changed some webpack configuration files. This can break production builds. Did you remember to run a production build locally to verify they still work?

@marcaaron marcaaron requested a review from roryabraham May 14, 2021 00:03
@marcaaron marcaaron marked this pull request as ready for review May 14, 2021 00:03
@marcaaron marcaaron requested a review from a team as a code owner May 14, 2021 00:03
@MelvinBot MelvinBot requested review from thienlnam and removed request for a team May 14, 2021 00:03
@botify
Copy link

botify commented May 14, 2021

Hey, I noticed you changed some webpack configuration files. This can break production builds. Did you remember to run a production build locally to verify they still work?

Copy link
Contributor

@roryabraham roryabraham left a comment

Choose a reason for hiding this comment

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

GH actions code seems fine!

@shawnborton
Copy link
Contributor

This is probably not the best place to deliver this feedback, but what is the difference between default and large for the header text size? I actually think large (assuming it's 17) is probably the default here. In fact, I think the only other time we would have a non-17 sized font in the header area is for chats. And I think that's okay - it's almost like we aren't using a font-size for the header title but rather, we are using a component as the header title - with the component being the little avatar(single, two, or counter) + name(s) component we use.

@marcaaron
Copy link
Contributor Author

Oh hmm yeah that's a good point @shawnborton. Looking through the code now and I don't really see anything passing this prop at all. I think all of the headers are set to have "large" text.

@marcaaron
Copy link
Contributor Author

@marcaaron
Copy link
Contributor Author

e.g. the IOU view is not using the large text

Screen Shot 2021-05-14 at 7 33 40 AM

Screen Shot 2021-05-14 at 7 33 31 AM

@shawnborton
Copy link
Contributor

Ah got it, the IOU view should be using the larger text size. I say we remove it as a prop and just update the default header styles everywhere to use 17px as the font size unless a component replaces the header title.

@marcaaron
Copy link
Contributor Author

marcaaron commented May 14, 2021

We could probably change this to textSize="small" and have the default be 17 if that makes more sense.

roryabraham
roryabraham previously approved these changes May 14, 2021
Copy link
Contributor

@roryabraham roryabraham left a comment

Choose a reason for hiding this comment

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

Just tested locally and this is AWESOME!!! I'm stoked for this – is there a tracking issue to create stories for all our components? 😁

@marcaaron
Copy link
Contributor Author

Fixed up that Header issue @shawnborton and added some stories for the more complex HeaderWithCloseButton. It shows off the variations:

2021-05-14_08-48-37.mp4

@shawnborton
Copy link
Contributor

Woo, thanks Marc!

@thienlnam
Copy link
Contributor

Wowwww this is so cool

Screen Shot 2021-05-14 at 1 04 14 PM

@marcaaron marcaaron requested a review from roryabraham May 14, 2021 21:29
Copy link
Contributor

@roryabraham roryabraham left a comment

Choose a reason for hiding this comment

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

Cool!

@roryabraham roryabraham merged commit c85ded0 into main May 14, 2021
@roryabraham roryabraham deleted the marcaaron-addStorybok branch May 14, 2021 22:21
@OSBotify
Copy link
Contributor

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

@OSBotify
Copy link
Contributor

🚀 Deployed to staging in version: 1.0.46-1🚀

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

@isagoico
Copy link

@roryabraham Heyo! Are we able to test this? I tried navigating to https://staging.expensify.cash/docs and I was unable to access storybook.

@marcaaron
Copy link
Contributor Author

@isagoico can check this off as we'll need to fix this somewhere else but can access via /docs/index.html for now. It's not a blocker in any case.

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.

7 participants