Enhancement/2940 migrate storybook #3230
Conversation
|
Size Change: +1.23 kB (0%) Total Size: 1.3 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Overall this looks awesome, thanks 👍🏻
A few notes:
- Looks like there's a linting error for the
useEffecthook inWithRegistrySetup, but should be easy-to-fix. - I know this isn't mentioned in the doc or the ACs/IBs, but think we can take this opportunity to differentiate the naming of story states better, starting here. The "Loaded" and "Loading" states are very different but their names are only a few letters apart; it's really hard to visually distinguish them in the list:
Why don't we try something like "Ready" or "Available" instead of "Loaded" as the prefix for those stories? I think that'd make it way easier to read.
Thoughts @felixarntz, @aaemnnosttv, @eugene-manuilov?
@tofumatt sounds good to me. I think |
There was a problem hiding this comment.
Thanks @asvinb ! I left a few comments regarding the current implementation. It looks like there is room to make these a bit more composable but not sure if there is a common decorator we can extract for all widgets in the Analytics module. If composition is easy without being overly verbose, we don't need to add many additional layers of abstraction.
I'm going to take a look at a potential solution for helping with the composition here, but this is off to a good start 👍
assets/js/modules/analytics/components/dashboard/DashboardUniqueVisitorsWidget.stories.js
Outdated
Show resolved
Hide resolved
assets/js/modules/analytics/components/dashboard/DashboardUniqueVisitorsWidget.stories.js
Outdated
Show resolved
Hide resolved
assets/js/modules/analytics/components/dashboard/DashboardUniqueVisitorsWidget.stories.js
Outdated
Show resolved
Hide resolved
assets/js/modules/analytics/components/dashboard/DashboardUniqueVisitorsWidget.stories.js
Outdated
Show resolved
Hide resolved
assets/js/modules/analytics/components/dashboard/DashboardUniqueVisitorsWidget.stories.js
Outdated
Show resolved
Hide resolved
assets/js/modules/analytics/components/dashboard/DashboardUniqueVisitorsWidget.stories.js
Outdated
Show resolved
Hide resolved
|
@asvinb @tofumatt I pushed a branch to test my ideas about exposing functions to select args so that the underlying component maintains ownership of that so that we don't need to duplicate them in story definitions. Some bits are a bit rough/unfinished (missing docs/etc) but it seems like an improvement. I also added a little utility for generating a mock response and receiving the report for Analytics with the given options to make that a little cleaner as well. Let me know what you think: enhancement/2940-migrate-storybook...poc/2940-alternate-widget-storybook |
|
@aaemnnosttv I like the approach, that seems like a good refactoring 👍🏻 Let's run with that idea for this—I can see how it'd be useful when we have multiple stories here. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
1 similar comment
This comment has been minimized.
This comment has been minimized.
|
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
|
@googlebot I consent. |
|
@asvinb I pushed a few commits here to make a few corrections but also to add some important functionality which was missing in the global decorators related to enabled features (which we recently changed) so I think this is just about good to go, but I'd like to get @tofumatt, @felixarntz, and @eugene-manuilov to have a look over as well since this is setting some important precedents. Basically, |
There was a problem hiding this comment.
The new features parameters approach looks good to me. As discussed with @aaemnnosttv I think it'd be good to have an example of a story that relies on the features parameters here, but that's all!
There was a problem hiding this comment.
Going to approve this, however, I can't say that I like the new implementation of the DashboardUniqueVisitorsWidget component 🤔. I understand why it has been changed that way, but IMO we sacrificed readability and maintainability for getting new storybooks working.
From my point of view, we should have separated the component into two: one for fetching data and passing it to another one, and one for rendering data passed from the first component. Then we would easily create stories for the second component to visualize it and would avoid a need to replicate the datastore environment for it.
Co-authored-by: Matthew Riley MacPherson <hi@tofumatt.com>
There was a problem hiding this comment.
I agree with @eugene-manuilov's assessment, and wanna emphasize here that we shouldn't need to change the component to get the new CSF stories set up. The approach of manually passing the request data for the selector and generated response data based on it should be maintained like we have been doing it in the old stories.
| import parseDimensionStringToDate from '../../util/parseDimensionStringToDate'; | ||
| import { isZeroReport } from '../../util'; | ||
| import { generateDateRangeArgs } from '../../util/report-date-range-args'; | ||
|
|
||
| const { useSelect } = Data; | ||
|
|
||
| function DashboardUniqueVisitorsWidget( { WidgetReportZero, WidgetReportError } ) { |
There was a problem hiding this comment.
This file shouldn't be renamed (back), as this was intentionally done as part of #3064. I guess that originally PR missed renaming the component itself, so let's do that here then.
| } ), | ||
| }; | ||
| }; | ||
|
|
There was a problem hiding this comment.
I agree with @eugene-manuilov, we shouldn't change this production code just to get the Storybook story setup simpler. Having exported functions in the same file as the component isn't good practice, so we shouldn't do that.
Let's provide the report options in Storybook manually the same way we do in the legacy versions of the story. This issue is not about changing that, it's only about migrating to the new proper way of CSF.
|
@aaemnnosttv I just saw your comment in #3230 (comment), I don't think I agree though. We can certainly look into making some of this data more usable, but throwing those functions into the same file as the component and making the component code harder to understand is not worth the benefit of not duplicating the object with the data. I suggest we keep it simple and focus on only the actual migration of this to CSF, rather than also changing the way we provide data for the selectors which wasn't really in scope here. |
|
Thanks for the feedback @felixarntz and @eugene-manuilov – I'll work on getting this straightened out tomorrow 👍 |
…igrate-storybook. # Conflicts: # assets/js/modules/index.test.js
| "@storybook/addon-viewport": "^6.2.9", | ||
| "@storybook/react": "^6.2.9", |
There was a problem hiding this comment.
Not sure if this is necessary or not, but Storybook would get stuck in a HMR loop when starting on the previous version. Technically out of scope here but given the minor change (and problem that it fixes for me) I think it's okay.
| }; | ||
| } ); | ||
|
|
||
| const WidgetWithComponentProps = withWidgetComponentProps( 'widget-slug' )( DashboardSearchVisitorsWidget ); |
There was a problem hiding this comment.
Maybe it would be more appropriate for this to be the real widget slug instead? Not really an issue though.
There was a problem hiding this comment.
My preference is to use dummy values when it isn't necessary/significant. It's also one less place to change if the slug were to change 😄
Summary
Addresses issue #2940
Relevant technical choices
DashboardSearchVisitorsWidget.jstoDashboardUniqueVisitorsWidget.jssince the latter exported theDashboardUniqueVisitorsWidgetcomponent.WidgetAreaRendereris being used in individual stories decorators as opposed to component decorators. No data flows in if it's the latter.*.stories.jsfilesChecklist