Closed razvanpapadopol closed 3 years ago
In order to fully extract the Design Picker to a separate package (@automattic/design-picker
) , the following files/components need to be extracted too:
To a new package @automattic/calypso-a11y
(which needs to be created):
client/lib/a11y
To @automattic/components
:
calypso/components/jetpack-logo
To @automattic/onboarding
:
/components/badge
To @automattic/design-picker
:
Design
typeavailable-designs.ts
available-designs-config.json
/components/mshots-image
Update: I spent today working on this task, and I reached a point where my changes were getting a bit out of control.
I think that we under-estimated the complexity of this extraction (we gave it an 8, but I believe it's at least a 13, if not a 21 depending on how proper we want this extraction to be):
The calypso/components/jetpack-logo
component is heavily used across the codebase, and its extraction to @automattic/components
should be done as a separate PR
In order to extract the JetpackLogo
component, we'd first need to extract client/lib/a11y
to a separate new package (@automattic/calypso-a11y
)
The Design
type requires the FontPair
type, which is still in client/landing/gutenboarding/constants.ts
— which probably doesn't belong to the @automattic/design-picker
component, since it's used mainly outside the design picker component. They are both stored in the automattic/onboard
store, which is also a candidate for extraction to @automattic/data-stores
— extracting the onboard store to the data-stores
package would feels like the cleanest solution
The available-designs.ts
and the available-designs-config.json
can be extracted to the @automattic/design-picker
component:
client/landing/gutenboarding/index.tsx
too?It feels like designs-related calls to the onboard data-store (setSelectedDesign
, getSelectedDesign
, hasPaidDesign
, getRandomizedDesigns
) should be moved to the @automattic/design-picker
too
Extracting the Badge
component to the @automattic/onboarding
package seems like an easy task, but should be probably done as a separate PR
Extracting /components/mshots-image
seems like an easy task
In the light of the above, I propose to split this issue into these smaller issues//PR:
client/lib/a11y
to a separate new package (@automattic/calypso-a11y
)<JetpackLogo />
to @automattic/components
<Badge />
component to the @automattic/onboarding
automattic/onboard
store to the @automattic/data-stores
package (which would extract the Design
type and solve the problem of its dependency from the the FontPair
type). Alternatively, find a solution to de-couple the Design
type from the FontPair
type<DesignPicker />
component to a separate new package (@automattic/design-picker
). This would require:
available-designs.ts
, available-designs-config.json
and potentially the randomisation logic (see above)<MShotsImage />
component<DesignPicker />
component
<DesignPicker />
support different themesThis current issue could serve as no. 5, while we would need to open new issues for the remaining points.
Thanks, @ciampo, for this thorough analysis.
When working on this we should keep in mind that we want a component (or package) that is decoupled from any stateful data structure (eg: Onboard
store) and has a simple API so it can be used in other contexts as well (more importantly, in /start
Onboarding flow at the moment). So I'd still propose the API from https://github.com/Automattic/wp-calypso/pull/51226 to which we can add the theme
.
locale: string;
onSelect: ( design: Design ) => void;
designs?: Design[];
isGridMinimal?: boolean;
While most of your points look valid, here are a few clarifications:
The Design type requires the FontPair type, which is still in client/landing/gutenboarding/constants.ts — which probably doesn't belong to the @automattic/design-picker component, since it's used mainly outside the design picker component.
It seems like FontPair
is being used also directly with the DesignSelector when setting default fonts for a design. See https://github.com/Automattic/wp-calypso/blob/trunk/client/landing/gutenboarding/onboarding-block/design-selector/index.tsx#L87
Since FontPair
type describes the fonts
property of the Design objects found in the file available-designs-config.json
that we are extracting, it should be extracted as well to the package and then imported where it's needed.
They are both stored in the automattic/onboard store, which is also a candidate for extraction to @automattic/data-stores
As we previously discussed, the Onboard
store belongs to Gutenboarding and it handles user selection and progress during the onboarding. @automattic/data-stores
package (see Readme) is used to fetch data from various WordPress.com REST API endpoints so the only stateful information stored there should be about data fetching (request, response, loading, error). Actually, we should also take the Launch
store out of @automattic/data-stores
https://github.com/Automattic/wp-calypso/issues/51363.
It feels like designs-related calls to the onboard data-store (setSelectedDesign, getSelectedDesign, hasPaidDesign, getRandomizedDesigns) should be moved to the
@automattic/design-picker
too
From these, the first 3 are tightly embedded in the consumer app (Gutenboarding) so I'd say we could plan to move only getRandomizedDesigns
in a follow-up. For now, Gutenboarding could continue to handle this locally by importing the result of available-designs.ts
from @automattic/design-picker
and then passing designs
as a prop to DesignPicker
.
Now, about the next steps:
Badge
component and keep a low priority for it since we're not actually using that badge at the moment, even in Gutenboarding. See https://github.com/Automattic/wp-calypso/issues/44239 that will block the new issue. Of course, in order to maintain the current possible variations of the UI, we can pass Badge
component from Gutenboarding, as a prop (LE: prop is called premiumBadge
in #51306).FontPair
and Design
type to the new @automattic/design-picker
package as part of (5).Onboard
store. TODO:
Create new issue for 1-3, add our findings (for each section of the issue), and mention that when triaging, it will need to be split into smaller issues
[Part of current draft PR]: update package.json deps + update generate screenshots scripts
Follow-up cleanup PR with non-breaking changes:
available-designs-config.json
and update accordingly (code comments, p2 posts, fieldguide pages)Next steps (separate issue):
<DesignPicker />
does not pass a list of designs, the package would know how to show the designs itself, plus another flag for randomising props)Timeline:
What
Extract design grid from
DesignSelector
step component in Gutenboarding.Why
We need to re-use the component in other flows.
A/C
DesignPicker
component should work anywhere in Calypso without a dependency on the Onboard store inclient/landing/gutenboarding
./new
design picking and site creation should continue to work exactly as before.Follow-up
DesignPicker
themeable (start / new onboarding). https://github.com/Automattic/wp-calypso/issues/51401Bonus points for extracting the component to an
@automattic
package. Discussion about the dependencies that needs extracting: p1616496332043300/1616488729.039900-slack-C0KDTA48Y and p1616512365044500/1616488729.039900-slack-C0KDTA48Y