sensei
sensei copied to clipboard
Automattic
Add interactivity to lesson steps
Resolves #7478 Based off of #7535
Proposed Changes
Testing Instructions
- Check that https://github.com/Automattic/sensei/pull/7535 continues working properly.
New/Updated Hooks
Deprecated Code
Pre-Merge Checklist
- [ ] PR title and description contain sufficient detail and accurately describe the changes
- [ ] Acceptance criteria is met
- [ ] Decisions are publicly documented
- [ ] Adheres to coding standards (PHP, JavaScript, CSS, HTML)
- [ ] All strings are translatable (without concatenation, handles plurals)
- [ ] Follows our naming conventions (P6rkRX-4oA-p2)
- [ ] Hooks (p6rkRX-1uS-p2) and functions are documented
- [ ] New UIs are responsive and use a mobile-first approach
- [ ] New UIs match the designs
- [ ] Different user privileges (admin, teacher, subscriber) are tested as appropriate
- [ ] Legacy courses (course without blocks) are tested
- [ ] Code is tested on the minimum supported PHP and WordPress versions
- [ ] User interface changes have been tested on the latest versions of Chrome, Firefox and Safari
- [ ] "Needs Documentation" label is added if this change requires updates to documentation
- [ ] Known issues are created as new GitHub issues
WordPress Dependencies Report
The github-action-wordpress-dependencies-report action has detected some script changes between the commit da17671396206b183fcfdbb224d403fd58ada535 and trunk. Please review and confirm the following are correct before merging.
| Script Handle | Added Dependencies | Removed Dependencies | Total Size | Size Diff |
|---|---|---|---|---|
blocks/quiz/index.js |
31.2 kB | +6 B ( +0.02% 🔼 ) | ||
admin/tour/course-tour/index.js |
wp-block-editor, wp-editor |
36.3 kB | +285 B ( +0.8% 🔼 ) | |
admin/tour/lesson-tour/index.js |
wp-block-editor, wp-edit-post, wp-editor |
36.5 kB | +2.3 kB ( +6.73% 🔼 ) |
This comment was automatically generated by the github-action-wordpress-dependencies-report action.
![github-actions[bot] avatar](https://avatars.githubusercontent.com/in/15368?v=4 "Published by a pub.dev verified publisher"){kind=small}
Codecov Report
Attention: Patch coverage is 38.15789% with 94 lines in your changes are missing coverage. Please review.
Project coverage is 51.78%. Comparing base (
05d19fa) to head (da17671). Report is 52 commits behind head on trunk.
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
| Files | Coverage Δ | |
|---|---|---|
| assets/admin/tour/lesson-tour/index.js | 54.54% <ø> (ø) |
|
| assets/blocks/quiz/quiz-block/quiz-settings.js | 68.42% <0.00%> (ø) |
|
| assets/admin/tour/course-tour/steps.js | 1.85% <25.00%> (-98.15%) |
:arrow_down: |
| assets/admin/tour/helper.js | 80.00% <83.33%> (ø) |
|
| ...ets/admin/tour/components/sensei-tour-kit/index.js | 60.71% <50.00%> (-12.62%) |
:arrow_down: |
| assets/admin/tour/lesson-tour/steps.js | 32.23% <31.66%> (-67.77%) |
:arrow_down: |
... and 15 files with indirect coverage changes
Continue to review full report in Codecov by Sentry.
Legend - Click here to learn more
Δ = absolute <relative> (impact),ø = not affected,? = missing dataPowered by Codecov. Last update d10be87...da17671. Read the comment docs.
![codecov[bot] avatar](https://avatars.githubusercontent.com/in/254?v=4 "Published by a pub.dev verified publisher"){kind=small}
Thank you for the review, @Imran92! I added a fix for the points 1 and 2: https://github.com/Automattic/sensei/pull/7538/commits/24de0884cea87aa1fb258801b9b28777f8efda45
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.
https://github.com/Automattic/sensei/assets/876340/979dbb34-c88b-411c-81a0-51172efa700a
As 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).
@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.
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-
The commits with the changes are: https://github.com/Automattic/sensei/pull/7538/commits/459a817fd324958414612cd0c35b6ada01a40f35 https://github.com/Automattic/sensei/pull/7538/commits/62e0b563574edf1b730d80081c0267e5b6e6008b https://github.com/Automattic/sensei/pull/7538/commits/5248885e647fed71911c83a681a6c50288830fb7
@Imran92, for the other 2 items:
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.
Implementation changed in: https://github.com/Automattic/sensei/pull/7538/commits/a1255404d40cdb78af1223dc4d64cb92d23eb4df
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.
I wrote tests to cover the cases there in this commit: https://github.com/Automattic/sensei/pull/7538/commits/0538655ba686be863f0b6744a2d574a8532175bf
Good catch, @Imran92! I didn't realize that.
I fixed it with https://github.com/Automattic/sensei/pull/7538/commits/da17671396206b183fcfdbb224d403fd58ada535, 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.