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

Storybook #509

Merged
merged 20 commits into from
May 24, 2022
Merged

Storybook #509

merged 20 commits into from
May 24, 2022

Conversation

illume
Copy link
Contributor

@illume illume commented Nov 10, 2021

Storybook

This creates stories for many of the frontend components. Along the way this makes some improvements and fixes some issues discovered.

cd frontend && npm install && npm run storybook

nebraska-storybook-screenie

Here is a summary of changes:

  • Typing issues were fixed whilst converting from JavaScript specs to TypeScript stories. Providing example input to components stresses the types more allowing the typechecker to find more issues.
  • Multiple different states are added for the stories, not just one. For each component you should be able to see different states the component can be in now inside the storybook. This also increases the test/type coverage.
  • Component interface documentation. Many of the component prop interfaces are documented where it makes sense. (So they are shown on hover in IDE and inside storybook).
  • Replaces spec tests which were mostly 'does it crash when rendering' and tests with storyshot snapshot tests. We get snap shot testing for free with storyshots.
  • Uses ComponentName.tsx filename for ComponentName. (Following react convention. It's hard to tell which <Item> in the react dev tools, and finding the component name can be quicker if it is the filename. If you have several Item or Index open in your editor, then each tab grows longer to include the path and it's hard to see which is which).

nebraska-ChannelItem
nebraska-find-item
nebraska-find-item-tabs

  • Separates Pure presentational components with state and effects mostly in a higher level container/wrapper component. See more about this pattern in the storybook presentational pages documentation. This reduces the complexity of larger components especially, because it divides the logic into two pieces.
  • Uses Context for some API modules to allow reliably mocking, and make the stores be initialized when used not when imported.
  • Removes redundant js folder (also for consistency with headlamp and other CRA apps). Also follow Headlamp a bit with lower case folder names ("common" and "layouts") which are not components so should not be capitalized.
  • Moves locale based date time handling into i18n/dateTime and make date use consistent TZ and formatting defaults under test (different platforms can have different default locales and TZ).
  • Adds icons to repo. (because it's inconvenient with formatting and git, and there's no reason really to generate them all the time). Going forward svg files should live inside their component folder.
  • Adds frontend coverage testing to make test, and to CI. The statement test coverage percentage is currently 35% (up from 25% before this PR).

How to use

To run the storybook:

cd frontend
npm install
npm run storynook

To test with the storybook:

cd frontend
npm install
npm run test

Testing done

  • no new console error logs in when running the frontend
  • no console error logs in each storybook component
  • no network connections made from the storybook

@illume illume changed the base branch from main to proof-of-concept3 January 4, 2022 22:21
@illume illume force-pushed the storybook branch 4 times, most recently from 19c21b7 to 2abd281 Compare January 7, 2022 21:19
@illume
Copy link
Contributor Author

illume commented Jan 10, 2022

This is rebased on the new proof-of-concept3 branch(which is itself up to date with main). Added a few more bug fixes in there related to timezones mostly along the way.

@yolossn yolossn force-pushed the proof-of-concept3 branch 2 times, most recently from 57b5238 to 0defa3d Compare January 21, 2022 11:13
Base automatically changed from proof-of-concept3 to main February 2, 2022 13:19
@illume illume force-pushed the storybook branch 2 times, most recently from 171ea4d to 53be272 Compare February 2, 2022 20:30
@illume illume force-pushed the storybook branch 3 times, most recently from 35f4cc1 to 134fb10 Compare February 22, 2022 23:41
@illume illume marked this pull request as ready for review February 23, 2022 00:12
@illume illume changed the title WIP: Storybook Storybook Feb 23, 2022
Copy link
Collaborator

@joaquimrocha joaquimrocha left a comment

Choose a reason for hiding this comment

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

Left a few comments. I would like to understand why we should repeat the context/area as part of the component names when we have them in the folder.

.gitignore Outdated Show resolved Hide resolved
frontend/src/js/icons/nebraska-logo.json Outdated Show resolved Hide resolved
frontend/src/App.tsx Show resolved Hide resolved
frontend/src/lib/themes.ts Show resolved Hide resolved
frontend/src/__tests__/Channels/Item.react.spec.js Outdated Show resolved Hide resolved
backend/api/spec.yaml Show resolved Hide resolved
@illume illume marked this pull request as draft February 23, 2022 15:14
@illume illume marked this pull request as ready for review March 1, 2022 13:32
@illume illume force-pushed the storybook branch 2 times, most recently from 623448d to 058947b Compare May 10, 2022 21:44
@illume
Copy link
Contributor Author

illume commented May 11, 2022

This was rebased against main, and is ready to review again.

@illume illume requested a review from ashu8912 May 11, 2022 09:41
Copy link
Collaborator

@joaquimrocha joaquimrocha left a comment

Choose a reason for hiding this comment

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

I still don't love the import ConfirmationContent from '../../components/Common/ConfirmationContent'; ->
import ConfirmationContent from '../../components/common/ConfirmationContent/ConfirmationContent
changes and think that if we had introduced also an index.tsx for it, we'd avoid having to modify so many files + having the repetition on the imports.

illume added 19 commits May 18, 2022 05:25
This is the default framework folder structure.
Gives consistency with other CRA apps like Headlamp.

Whilst renaming, we move components in common/ and layout/ into their
own folder. So all the files related to them can be all together.
Themes should work as before.
All the theme selection features are not completely integrated
yet, but it is enough to use the themes within storybooks.
The Card.stories.tsx is minimal and unfinished because storyshots
requires one test to be there for the test to pass.
- Named files after component (ChannelItem.tsx not Item.tsx)
- Fixed some types to match app data
- Fixed some other type issues found
This is because different systems have different default locales
and different timezones - which makes testing more difficult.

Moved date/time formatting functions out of helpers into
src/i18n/dateTime.

The cross-env package is for cross platform setting of env vars.
- Removed __tests__/Activity (covered by storyshots)
- Named files after component (ActivityItem.tsx not Item.tsx)
- Fixed some types to match app data
- Fixed some other type issues found
- Added id to Activity API response
- Fixed issue with key using number instead of unique id
- Material UI generated IDs are not stable, mock for storyshots
- Used toLocaleString to make date locale related tests happy
- Removed __tests__/Applications(covered by storyshots)
- Named files after component (ApplicationItem.tsx not Item.tsx)
- Fixed some types to match app data
- Fixed some other type issues found
- Made ApplicationItemGroupItem use APIContext so stories can mock API
- Named files after component (GroupItem.tsx not Item.tsx)
- Separated some components into their own files

backend:
- version_breakdown was returning null instead of [] when empty
The coverage percentages in package.json that the testing needs to
meet are the current levels.
@illume
Copy link
Contributor Author

illume commented May 18, 2022

I filled all the common/ and layout/ folder components with index.tsx.

@illume illume requested a review from joaquimrocha May 23, 2022 09:48
@joaquimrocha joaquimrocha merged commit 271735e into main May 24, 2022
@joaquimrocha joaquimrocha deleted the storybook branch May 24, 2022 12: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