-
Notifications
You must be signed in to change notification settings - Fork 36
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
Upgrade to Storybook 7 #726
Upgrade to Storybook 7 #726
Conversation
@@ -8,6 +8,5 @@ | |||
"lib": ["es2019", "dom"], | |||
"skipLibCheck": true | |||
}, | |||
"include": ["src/**/*"], | |||
"exclude": ["src/__tests__/**/*"] |
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.
This exclude was overriding the exclude from the extended config at the root level
d12147b
to
0385d4a
Compare
025b352
to
93f5fc6
Compare
93f5fc6
to
fe23cf6
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.
Really excited about this. Great work
export const RouteColorBackground = () => ( | ||
<TransitVehicleOverlay | ||
IconContainer={withRouteColorBackground(DefaultIconContainer)} | ||
ModeIcon={TriMetModeIcon} | ||
vehicles={vehicles} | ||
/> | ||
); | ||
|
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.
Why was this removed
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.
bad merge maybe?
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.
Could you check consistency in using the story parameters to disable storyshots? Sometimes they are applied component wide, other times they are applied for each story.
"nock": "^11.7.0", | ||
"prettier": "^1.19.1", | ||
"puppeteer": "^10.2.0", | ||
"react": "^17.0.2", | ||
"react-dom": "^17.0.2", | ||
"react": "^18.2.0", |
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.
Does that mean that OTP-RR will have to use React 18 once merged?
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 think so, good point. I'll get that PR ready.
@@ -20,13 +20,13 @@ | |||
}, | |||
"dependencies": { | |||
"@opentripplanner/base-map": "^3.1.0", | |||
"@opentripplanner/core-utils": "^11.4.0" | |||
"@opentripplanner/core-utils": "^11.4.1" |
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.
Note that this is to accommodate tests only, and that no release will be triggered for this package.
@@ -21,15 +21,15 @@ | |||
"dependencies": { | |||
"@mapbox/polyline": "^1.1.0", | |||
"@opentripplanner/base-map": "^3.1.0", | |||
"@opentripplanner/core-utils": "^11.4.0", | |||
"@opentripplanner/core-utils": "^11.4.1", |
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.
Note that this is to accommodate tests only, and that no release will be triggered for this package.
@@ -954,7 +954,7 @@ const LocationField = ({ | |||
</S.HiddenContent> | |||
<ItemList | |||
// Hide the list from screen readers if no enabled options are shown. | |||
aria-hidden={hasNoEnabledOptions} | |||
aria-hidden={hasNoEnabledOptions || !menuVisible} |
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 noted the fix
commit for this change, which will trigger a release for this package.
@@ -10,7 +10,7 @@ | |||
"license": "MIT", | |||
"private": false, | |||
"dependencies": { | |||
"@opentripplanner/core-utils": "^11.4.0", | |||
"@opentripplanner/core-utils": "^11.4.1", |
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.
Note that this is to accommodate tests only, and that no release will be triggered for this package.
@@ -121,7 +124,7 @@ class LegDiagramPreview extends Component<Props, State> { | |||
</S.PreviewDiagramElevationLoss> | |||
</S.PreviewDiagramTitle> | |||
{profile.points.length > 0 ? ( | |||
generateSvg(profile, width) | |||
generateSvg(profile, IS_TEST_RUNNER ? "245" : width) |
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.
Note that this is to accommodate tests only, and that no release will be triggered for this package.
Thanks for the feedback on consistency of disabling storyshots. I have moved all of them into the default export and got rid of the disableStoryshots variables since they're only used in one place now. |
c475311
to
e951978
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.
Going ahead and approving. That's a needed upgrade.
@@ -43,7 +41,8 @@ function CatDogIcon({ type }: UserLocationAndType) { | |||
export default { | |||
component: EndpointsOverlay, | |||
decorators: [withMap()], | |||
title: "EndpointsOverlay" | |||
title: "EndpointsOverlay", | |||
parameters: { storyshots: { disable: true } } |
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.
Nit: Sort attributes
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.
oh oops, I forgot to do this.
f4186a4
into
opentripplanner:master
This PR upgrades Storybook to Storybook 7, along with a host of other related upgrades.
First of all, dates. We can't rely on current time or the computer's timezone to render stories, since those vary.
There was also an issue with the elevation profile view, which uses an auto resize component to grab the page width. For some reason this was rendering differently in CI, likely due to differences between the headless browser used on Linux by Playwright and on MacOS. To solve this, I check the user agent and when we are in Playwright, we can mock this value and fix the width to a constant. This is in
leg-diagram-preview.tsx
.AXE tests have also changed. The latest way to do this is with AXE-Playwright, which just runs in the Playwright tests. These tests have detected many more errors than before, which is causing the tests to fail. I don't know if these are false positives or if we had tests disabled before, or if the new checker has more rules.
The final issue that I still don't really know the cause of was that we had build problems when the default export of a component was different from the file name itself. We had many icons where the name of the icon began with Svg, while the filename did not. For example,
export default SvgCar
inCar.js
. Webpack 5 (the new build system with Storybook 7). I fixed this by renaming the exports in all these files. I wish I had been able to find why this wasn't working, since I believe it should be.For future reference, this is how I accomplished this rename: