-
Notifications
You must be signed in to change notification settings - Fork 45
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Storybook #509
Storybook #509
Conversation
19c21b7
to
2abd281
Compare
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. |
0981499
to
f959fcb
Compare
57b5238
to
0defa3d
Compare
cf72459
to
dbeed12
Compare
171ea4d
to
53be272
Compare
35f4cc1
to
134fb10
Compare
There was a problem hiding this 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.
frontend/src/components/Groups/GroupCharts/StatusCountTimeline.stories.tsx
Show resolved
Hide resolved
623448d
to
058947b
Compare
This was rebased against main, and is ready to review again. |
There was a problem hiding this 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.
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.
from Headlamp.
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.
I filled all the common/ and layout/ folder components with index.tsx. |
Storybook
This creates stories for many of the frontend components. Along the way this makes some improvements and fixes some issues discovered.
Here is a summary of changes:
<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).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.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:
Testing done