-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Customer Home: Add Mastering Gutenberg Card #39617
Customer Home: Add Mastering Gutenberg Card #39617
Conversation
Just a couple quick questions (cc @danhauk, @gwwar)
|
Should "Block Editor" be lower case? These graphics from core iteration on welcome modal might be useful: WordPress/gutenberg#19209 (comment) |
I'm going to mark this for review now even though it's not technically finished, but I can't really proceed much further without some of the advice with the questions above. :) Proposed Tutorials Adding, moving and deleting blocks: https://cdn.discordapp.com/attachments/449249666798714890/680849856897745121/adjusting-blocks-tutorial.mp4 Quickly navigating between blocks: https://cdn.discordapp.com/attachments/449249666798714890/681109889686175746/block-navigation-tutorial.mp4 Adding links: https://cdn.discordapp.com/attachments/449249666798714890/681109899580669974/linkstutorial.mp4 Using images: https://cdn.discordapp.com/attachments/449249666798714890/681115972975067179/using-images-tutorial.mp4 Let me know if these videos are okay. The topics seem to cover the main issues in the forums: https://en.forums.wordpress.com/topic-tag/block-editor/ |
This is looking great @Aurorum <3 I'll leave design and such to others, but a couple of quick notes.
Side note that our designers are at a meetup this week, so review will be delayed for a while. Thanks for your patience, we appreciate the contributions! 👍 |
Oh, ignore me – they open modals 🙃 |
client/my-sites/customer-home/mastering-gutenberg-card/index.jsx
Outdated
Show resolved
Hide resolved
That border was in the design posted in the original issue. I thought it looked prettier, but it is odd seeing it on just this card. I'm unsure if the solution to that is removing it from this card or adding it to all the other cards.
I'm unsure if that's more consistent with the ones below, but these are indeed modals! :)
No worries, thanks for letting me know! |
Hi there @Aurorum, Customer Home designer here, thank you so much for all of your contributions and enthusiasm for this 😍. I don't happen to be at the same meetup as the rest of the design team, and I didn't want to leave you hanging any longer on design feedback and the questions you asked: Overall: Styling
Behavior
Layout
|
Seems it makes sense to start uploading videos directly in Calypso. I'll see if I can open a PR tomorrow adding a video loader to webpack but anyone is welcome to beat me to it (cc @sgomes). |
I started one in #39707. :) |
Okay, I think this should be ready for review now. I've updated the testing instructions to provide a bit more detail after the changes! :) |
|
||
export default connect( state => { | ||
return { | ||
cannotUseGutenberg: isClassicEditorForced( state, getSelectedSiteId( state ) ), |
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.
Good thinking here! 👍 We'll probably move this check over to the layout API but this is a great reminder ✨
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.
Should I remove it here now? Just checking as it seems other cards have display logic within them too. :)
if ( hideStats ) { | |
return null; | |
} |
wp-calypso/client/my-sites/customer-home/cards/features/grow-earn/index.jsx
Lines 32 to 34 in 1d38c53
if ( isEnabled( 'signup/wpforteams' ) && isSiteWPForTeamsProp ) { | |
return null; | |
} |
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.
We can follow up later once they are moved to the API, no need to remove them for now.
Note that isClassicEditorForced
will return false when the selected editor is the classic one. So I wonder if we should show the card only when the selected editor is Gutenberg: cannotUseGutenberg: getSelectedEditor( state, siteId ) === 'classic'
;
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.
We can follow up later once they are moved to the API, no need to remove them for now.
Actually, since the card won't be displayed until we update the API, let's remove the checks now. We'll make sure the card won't be returned by the API is Gutenberg is not the selected editor.
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.
So I wonder if we should show the card only when the selected editor is Gutenberg: cannotUseGutenberg: getSelectedEditor( state, siteId ) === 'classic'
There was some discussion about this here:
While I agree that it would make sense to hide the card if a person explicitly doesn't want to use Gutenberg, the community keeps adding new functionality and fixing things that didn't work well in a previous release. A Dismiss option works best for serving all of those intentions.
Now that it's just an ordinary card and not a dismissable one, I'm not sure that still applies.
I've removed the checks anyway. :)
client/my-sites/customer-home/cards/education/mastering-gutenberg/index.jsx
Outdated
Show resolved
Hide resolved
client/my-sites/customer-home/cards/education/mastering-gutenberg/index.jsx
Outdated
Show resolved
Hide resolved
Nice work @Aurorum ! This is looking great so far ✨ |
Just a heads up that I just merged #40937 which adds to the support article dialog the ability to scroll to an anchor, which effectively unblocks this PR. |
Thank you! I've updated the PR to use the support article dialog - much less code needed too! :) |
Actually, just want to check if "adjusting settings of blocks" and "customizing posts and pages with blocks" should still be the links. Although they apply better to the video, the previous links seem better suited to the content of the support doc. |
Since the video is the primary content that will be shown in the dialog, I think the copy of the links still match what the user will see in the dialog. |
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.
Almost there @Aurorum, great job as always! Overall this works great, but I think we need to flip the links.
Also noted a weird focused style disappearing when hovering a link, but not a blocker.
client/my-sites/customer-home/cards/education/mastering-gutenberg/index.jsx
Show resolved
Hide resolved
client/my-sites/customer-home/cards/education/mastering-gutenberg/index.jsx
Outdated
Show resolved
Hide resolved
client/my-sites/customer-home/cards/education/mastering-gutenberg/index.jsx
Outdated
Show resolved
Hide resolved
I think that's an artifact of us starting with an icon that had a play button originally. I think not triggering the dialog is okay here. We can tidy up the other card behavior in another PR. |
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.
Nice work @Aurorum ! ✨ Thanks for being patient with us on this one too! Overall I think this is great to land. We can add some other enhancements like adding analytics, and thinking through what we should do when we have too many educational cards in follow up PRs.
Desktop | Mobile | Article Open |
---|---|---|
![]() |
![]() |
![]() |
This Pull Request is now available for translation here: https://translate.wordpress.com/deliverables/3203786 Thank you @Aurorum for including a screenshot in the description! This is really helpful for our translators. |
Translation for this Pull Request has now been finished. |
Changes proposed in this Pull Request
This is very much a WIP; I still need to create the videos with some more detailed annotations/text, and select a thumbnail for them, along with making sure the support links are localised etc.
This adds another card with some Gutenberg resources.
Testing instructions
Copy and paste this into
client/my-sites/customer-home/locations/primary/index.jsx
A list of links should appear. The card should not appear for sites unable to use Gutenberg.
Clicking one should open a modal with a video and a link to a support document on it. Verify that both of these work (the video should autoplay).
The card should be dismissable, but hidden if a user cannot use Gutenberg (based on the
isClassicEditorForced
selector).Fixes #39132