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
Product Editor Onboarding: Show spotlight for first time visitors #38590
Conversation
…ng on the options in the future
Test Results SummaryCommit SHA: ff032b3
To view the full API test report, click here. To view the full E2E test report, click here. To view all test reports, visit the WooCommerce Test Reports Dashboard. |
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.
Overall nice work @nathanss, this was testing well, I just noticed one design discrepancy for the initial tour popup, with the missing purple box and the beta tag.
For the rest I left several comments within the code with little suggestions.
), | ||
}, | ||
heading: __( | ||
'Meet a streamlined product form', |
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 is missing the beta pill
if ( isTourOpen ) { | ||
return ( | ||
<TourKit | ||
config={ { |
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.
In the design the tourkit also includes a purple box at the top, is it possible to add this?
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.
For the background color in the TourKit, we would have to change the component to allow customizing the CardHeader, which is the part where the X button is, and where we would change the background color and increase its size.
Do you think I should implement this?
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.
Do you think I should implement this?
Yeah, but you can do this as part of a follow up.
); | ||
} else if ( isGuideOpen ) { | ||
return ( | ||
<Guide |
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 we move this into it's own component - block-editor-guide
? and use it within the block editor tour wrapper?
<Guide | ||
className="woocommerce-block-editor-guide" | ||
finishButtonText={ __( 'Close', 'woocommerce' ) } | ||
onFinish={ () => { |
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.
Hmmm it sucks there is only a single callback and their is no way of deciphering what step we are on.
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 agree. Can we ask for this feature for the component, perhaps?
background-color: #cfb9f6; | ||
} | ||
&__background4 { | ||
height: 220px; |
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 we turn the height into a variable here, incase we change it, and forget one of them.
} | ||
|
||
&__text { | ||
height: 80px; |
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.
Is this manual height necessary?
background-color: #bea0f2; | ||
} | ||
&.components-modal__frame { | ||
width: 320px; |
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.
Maybe we should use max-width
here instead?
|
||
export const useBlockEditorTourOptions = () => { | ||
const { updateOptions } = useDispatch( OPTIONS_STORE_NAME ); | ||
const [ isGuideOpen, setIsGuideOpen ] = useState( false ); |
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 local state would prevent us from re-triggering the guide from the options menu, as is needed in: #38522
Could we move this state into a useContext
?
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.
Hey...
I payed attention to that requirement.
That's why I split into BlockEditorTour and BlockEditorTourWrapper. I figured that the BlockEditorTour
component could be used directly from the About menu without using this hook, as this hook is more related to the option persisted in the back-end.
I made another refactor now and extracted the BlockEditorGuide component. I think we can just call the component directly and it will show up. Let me know if you find a problem with this approach.
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 made another refactor now and extracted the BlockEditorGuide component. I think we can just call the component directly and it will show up. Let me know if you find a problem with this approach.
That seems good to me :) That seems lower effort and easier then adding a whole context.
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.
Thanks for the updates @nathanss, this looks good, and tested well!
Nice work 🎉
Submission Review Guidelines:
Changes proposed in this Pull Request:
Show a spotlight and a guided tour for first time visitors, using TourKit and Guide components.
Bug with Guide component: the 'X' button from the modal keeps focusing when navigating between pages.
Limitation with Guide component: it's not possible to know if the Guide was closed or finished, because the same callback is called for both, with no parameters.
Closes #38516 .
How to test the changes in this Pull Request:
Using the WooCommerce Testing Instructions Guide, include your detailed testing instructions:
wcadmin_block_product_editor_spotlight_view
tracks event should be fired.wcadmin_block_product_editor_spotlight_dismissed
tracks event should be fired.woocommerce_block_product_tour_shown
using WCA test helper.wcadmin_block_product_editor_spotlight_completed
tracks event should be fired.