-
Notifications
You must be signed in to change notification settings - Fork 195
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
Add interactivity to lesson steps #7538
Conversation
WordPress Dependencies ReportThe
This comment was automatically generated by the |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## trunk #7538 +/- ##
============================================
- Coverage 51.92% 51.78% -0.15%
- Complexity 11265 11307 +42
============================================
Files 630 640 +10
Lines 47683 48115 +432
Branches 421 467 +46
============================================
+ Hits 24759 24914 +155
- Misses 22587 22822 +235
- Partials 337 379 +42
... and 15 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
/** | ||
* Performs step actions one after another. | ||
* | ||
* @param {Array} stepActions An array of selectors to highlight. | ||
*/ | ||
export async function performStepActionsAsync( stepActions ) { | ||
removeHighlightClasses(); | ||
clearTimeout( stepActionTimeout ); |
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 to make sure actions won't happen later after moving to different steps because of the setTimeout
. Based on the removeHighlightClasses();
call in the previous line, I suppose we can assume that the performStepActionsAsync
is a requirement in the actions, right?
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.
Yes I think so in general. Thanks for the improvement! Just added a comment about it here #7538 (comment)
Test the previous changes of this PR with WordPress Playground. |
@@ -24,9 +24,16 @@ import { WpcomTourKit } from '@automattic/tour-kit'; | |||
* @param {string} props.tourName The unique name of the tour. | |||
* @param {string} props.trackId ID of tracking event (optional). Tracking will be enabled only when provided. | |||
* @param {TourStep[]} props.steps An array of steps to include in the tour. | |||
* @param {Function} [props.beforeEach] A function to run before each step. |
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.
It adds a new feature to run something before each step. Similar to what we have in tests to reset or prepare something. In the future, if needed it could have some more events like 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.
Nice!
@@ -42,11 +42,17 @@ describe( 'SenseiTourKit', () => { | |||
Close | |||
</button> | |||
<button | |||
data-testid="nextButton" | |||
data-testid="stepViewOnceButton" |
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 renamed this one since it's tied to the onStepViewOnce
callback, and I added another one tied to the onNextStep
callback, so I could use it in the new test.
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.
Makes sense!
Test the previous changes of this PR with WordPress Playground. |
Test the previous changes of this PR with WordPress Playground. |
Test the previous changes of this PR with WordPress Playground. |
Test the previous changes of this PR with WordPress Playground. |
Thank you for the review, @Imran92! I added a fix for the points 1 and 2: 24de088 About the point 3, I noticed that it only happens if the block is already visible in your viewport but in a bad position (almost out). If it's completely out, Gutenberg will moving it to a good position. Screen.Recording.2024-03-15.at.16.15.47.movAs a workaround, I thought of adding a temporary fade in the tour, do the user sees what's behind, but the experience didn't look nice. So I'd consider it as an edge case and just ignore. WDYT? I'm a little afraid to try to manipulate the scroll and be more error-prone in the future (in Gutenberg updates, for example). |
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 addressing the comments! LGTM!
Just a couple of comments, but I'm approving already.
- I've noticed that we are inserting a True/False question still in case the user has a question of that type in the second or later position. Do you think it'll be better we can use the True/False question regardless of the position and insert only as a last resort? I don't have a strong opinion. Current behavior can be okay too.
- As we have a number of helper functions on top, do you think we should write some tests for just those functions? Not necessary, but can be good to have.
- This one feels a little important to me, because I missed the focus outline multiple times even after knowing it should be there. That is, for Quiz and Question settings, we are highlighting the sidebar. But currently, the outline is only on the left side. So it's not too clear what we are highlighting-
Screen.Recording.2024-03-18.at.4.21.34.PM.mov
Can we somehow focus on the inner section of the side panel?
So we can highlight it with the tour
@Imran92! I will still work on the other 2 items, but I already sent a purpose for the last one for you too see what you think.
![]() ![]() |
Test the previous changes of this PR with WordPress Playground. |
Test the previous changes of this PR with WordPress Playground. |
@Imran92, for the other 2 items:
Implementation changed in: a125540
I wrote tests to cover the cases there in this commit: 0538655 |
Test the previous changes of this PR with WordPress Playground. |
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 Renatho! Looks good and works nice!
There's only one small issue I've noticed as a side effect of the last changes. Feel free to work on it separately if you want to. I'm approving already
When we minimize a tour, we remove all the highlighting, just like GB tour's spotlight. I noticed that the suffixed highlights are not getting removed like that-
Screen.Recording.2024-03-19.at.5.55.38.PM.mov
Good catch, @Imran92! I didn't realize that. I fixed it with da17671, and I'm merging this PR. But a post-merge check is welcome, and I could fix anything in another PR if needed. While working on this, I just realized another bad thing, but I couldn't find any good solution for that. If you don't have any idea too, I think we could live with it. The only thing I could think of is trying to override that hover class to not add the hover style, but it seems very fragile to me, which could cause other issues on Gutenberg updates. ![]() |
Test the previous changes of this PR with WordPress Playground. |
Resolves #7478
Based off of #7535
Proposed Changes
beforeEach
to run before each action.Testing Instructions
Video
Screen.Recording.2024-03-14.at.15.41.55-2.mov
Pre-Merge Checklist