Skip to content

Conversation

@renatho
Copy link
Contributor

@renatho renatho commented Oct 29, 2024

Resolves #7692 #6123

Proposed Changes

Sorry for the huge PR, but all the things were depending on each other, so it's complicated to separate in small ones. Notice that this big commit is related to lint fixes: https://github.com/Automattic/sensei/pull/7695/commits/82ca947db8de772a4123db6d2ba5951d18934256t

  • Update dependencies.
  • Clean up some dependencies.
  • Fix lint.
  • Fix unit tests.
  • Fix dev configurations.
  • Fix styles.

Testing Instructions

  1. Build the project and make sure it works.
  2. Navigate through the Sensei LMS features (especially Import/Export), and make sure the styles continue working properly.
  3. Check that tests and lint are passing.

Students menu (#6123)

  1. Go to Sensei LMS » Students
  2. You must have a couple of students. Click on the 3 dots at the end of any student.
  3. Make sure the menu opens normally and the styles are not broken.

Sensei Pro

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

@renatho renatho self-assigned this Oct 29, 2024
@renatho renatho force-pushed the update/wordpress-dependencies branch from 427e4f8 to 41fe999 Compare October 29, 2024 21:15
@renatho renatho force-pushed the update/wordpress-dependencies branch from 41fe999 to 710033b Compare October 29, 2024 21:16
@Automattic Automattic deleted a comment from github-actions bot Oct 29, 2024
@Automattic Automattic deleted a comment from github-actions bot Oct 29, 2024
@github-actions
Copy link

Test the previous changes of this PR with WordPress Playground.

@github-actions
Copy link

Test the previous changes of this PR with WordPress Playground.

@github-actions
Copy link

Test the previous changes of this PR with WordPress Playground.

@github-actions
Copy link

Test the previous changes of this PR with WordPress Playground.

@github-actions
Copy link

github-actions bot commented Oct 30, 2024

WordPress Dependencies Report

The github-action-wordpress-dependencies-report action has detected some script changes between the commit 825da2f and trunk. Please review and confirm the following are correct before merging.

Script Handle Added Dependencies Removed Dependencies Total Size Size Diff
js/admin/course-edit.js 10.3 kB +403 B ( +4.09% 🔼 )
js/admin/course-index.js 697 B -54 B ( -7.2% ⬇️ )
js/admin/event-logging.js 662 B +13 B ( +2.01% 🔼 )
js/admin/lesson-bulk-edit.js 700 B +14 B ( +2.05% 🔼 )
js/admin/lesson-quick-edit.js 727 B +14 B ( +1.97% 🔼 )
js/admin/message-menu-fix.js 190 B +19 B ( +11.12% 🔼 )
js/admin/meta-box-quiz-editor.js 6.58 kB -49 B ( -0.74% ⬇️ )
js/admin/lesson-edit.js 2.46 kB -111 B ( -4.32% ⬇️ )
js/admin/lesson-ai.js 2.29 kB +399 B ( +21.09% 🔼 )
js/admin/ordering.js 359 B +15 B ( +4.37% 🔼 )
js/admin/sensei-notice-dismiss.js 1.11 kB -57 B ( -4.9% ⬇️ )
js/admin/custom-navigation.js 282 B +15 B ( +5.62% 🔼 )
js/admin/reports.js 747 B -87 B ( -10.44% ⬇️ )
js/frontend/course-archive.js 171 B +15 B ( +9.62% 🔼 )
js/frontend/course-video/video-blocks-extension.js 5.49 kB -180 B ( -3.18% ⬇️ )
js/file-upload-question-type.js 794 B -72 B ( -8.32% ⬇️ )
js/grading-general.js 2.65 kB -58 B ( -2.15% ⬇️ )
js/image-selectors.js 227 B +20 B ( +9.67% 🔼 )
js/learners-bulk-actions.js 1.37 kB +156 B ( +12.83% 🔼 )
js/learners-general.js 2.09 kB +14 B ( +0.68% 🔼 )
js/modules-admin.js 2.03 kB -5 B ( -0.25% ⬇️ )
js/ranges.js 442 B +15 B ( +3.52% 🔼 )
js/settings.js 1.2 kB +14 B ( +1.19% 🔼 )
js/admin/settings/experimental-features.js 306 B +17 B ( +5.89% 🔼 )
js/user-dashboard.js 192 B +15 B ( +8.48% 🔼 )
js/question-answer-tinymce-editor.js 845 B -35 B ( -3.98% ⬇️ )
setup-wizard/index.js 17.4 kB +865 B ( +5.23% 🔼 )
home/index.js 22.4 kB +1.11 kB ( +5.23% 🔼 )
data-port/import.js 15.1 kB +638 B ( +4.42% 🔼 )
data-port/export.js react 5.93 kB +469 B ( +8.6% 🔼 )
blocks/single-page.js 18.4 kB +691 B ( +3.91% 🔼 )
blocks/single-course.js 31.3 kB +943 B ( +3.11% 🔼 )
blocks/single-lesson.js 7.72 kB +319 B ( +4.32% 🔼 )
blocks/lesson-action-blocks.js 13.9 kB +275 B ( +2.03% 🔼 )
blocks/global-blocks.js 21.5 kB +309 B ( +1.47% 🔼 )
blocks/quiz/index.js 37.1 kB +2.29 kB ( +6.56% 🔼 )
blocks/quiz/ordering-promo/index.js react wp-element 1.46 kB +387 B ( +36.17% 🔼 )
blocks/shared.js 14 kB +159 B ( +1.15% 🔼 )
blocks/frontend.js 2 kB -148 B ( -6.9% ⬇️ )
blocks/email-editor.js 1 kB -41 B ( -3.92% ⬇️ )
admin/course-pre-publish-panel/index.js wp-element 4.42 kB +398 B ( +9.9% 🔼 )
admin/editor-wizard/index.js 13 kB +302 B ( +2.38% 🔼 )
admin/tour/course-tour/index.js 39.6 kB +410 B ( +1.05% 🔼 )
admin/tour/lesson-tour/index.js 36.7 kB +403 B ( +1.11% 🔼 )
admin/exit-survey/index.js react 2.89 kB +202 B ( +7.51% 🔼 )
admin/students/student-action-menu/index.js 5.23 kB +305 B ( +6.19% 🔼 )
admin/students/student-bulk-action-button/index.js 5.42 kB +390 B ( +7.77% 🔼 )
course-theme/learning-mode.js lodash 2.88 kB -3.27 kB ( -53.19% ⬇️ )
course-theme/course-theme.editor.js 1.66 kB -76 B ( -4.39% ⬇️ )
course-theme/blocks/index.js 14 kB +760 B ( +5.77% 🔼 )
course-theme/learning-mode-templates/index.js react 3.51 kB +359 B ( +11.41% 🔼 )

This comment was automatically generated by the github-action-wordpress-dependencies-report action.

@github-actions
Copy link

Test the previous changes of this PR with WordPress Playground.

@github-actions
Copy link

Test the previous changes of this PR with WordPress Playground.

@github-actions
Copy link

Test the previous changes of this PR with WordPress Playground.

@renatho renatho force-pushed the update/wordpress-dependencies branch from 3e6bfe9 to 6ca3cf5 Compare October 31, 2024 13:45
@github-actions
Copy link

Test the previous changes of this PR with WordPress Playground.

@github-actions
Copy link

Test the previous changes of this PR with WordPress Playground.

package.json Outdated
"react": "17.0.2",
"react-dom": "17.0.2",
"resolve-url-loader": "5.0.0",
"sass": "1.35.2",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The @wordpress/scripts installs other minor versions (it uses the ^), and sass introduced a lot of deprecations because its API is changing.

I checked that Gutenberg codebase is still using it in the old way, so I also kept it using the same approach by installing the specific version.

Copy link
Contributor Author

@renatho renatho Oct 31, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before checking how Gutenberg was doing it, I tried to fix the deprecations but it's risky to break something and it's a big amount of work. So it's better to wait for Gutenberg decisions before changing it in Sensei. Notice that we also import scss files directly from Gutenberg and Calypso (which use the deprecated @import).

<div className="sensei-notice__actions">
{ actions.map( ( action ) => (
<NoticeAction key={ action.url } action={ action } />
<NoticeAction key={ action.label } action={ action } />
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make sure the key will be always available since url is not always available.

@github-actions
Copy link

Test the previous changes of this PR with WordPress Playground.

@renatho renatho changed the title Update npm dependencies (especially @wordpress) Update npm dependencies Mar 24, 2025
@github-actions
Copy link

Test the previous changes of this PR with WordPress Playground.

@github-actions
Copy link

Test the previous changes of this PR with WordPress Playground.

@github-actions
Copy link

Test the previous changes of this PR with WordPress Playground.

* @param {Object[]} blocks Blocks to replace with the new content.
* @param {Object} replaces Object containing content to be replaced. The keys are the block
* classNames to find. The values are the content to be replaced.
* @param {Object} replaces Object containing content to be replaced. The keys are the block classNames to find. The values are the content to be replaced.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In Sensei LMS we have some custom plugin to apply a prettier fix to JSDoc.

It's broken for multiline now, so as a short-term fix, I just changed it to single line where needed.

@github-actions
Copy link

Test the previous changes of this PR with WordPress Playground.

@renatho renatho force-pushed the update/wordpress-dependencies branch from c4cea5c to 373c916 Compare March 25, 2025 20:55
@github-actions
Copy link

Test the previous changes of this PR with WordPress Playground.

@github-actions
Copy link

Test the previous changes of this PR with WordPress Playground.

@renatho renatho marked this pull request as ready for review March 25, 2025 21:41
@renatho renatho requested a review from a team March 25, 2025 21:41
@github-actions
Copy link

Test the previous changes of this PR with WordPress Playground.

@renatho renatho force-pushed the update/wordpress-dependencies branch from 4760307 to 825da2f Compare March 25, 2025 21:57
@github-actions
Copy link

Test the previous changes of this PR with WordPress Playground.

Copy link
Member

@m1r0 m1r0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've tested all major flows and it looks good! Amazing work, Renatho!

I've noticed just one small bug related to the lesson layouts, which could be addressed separately.
image

This is probably not related, but I've noticed a few deprecation warnings on the console.
image

Speaking of deprecation warnings, there are some warnings related to Sass when building the assets. This is not a blocker, but we should keep it in mind.
image

Again, thank you for working on this! 🙇

@m1r0
Copy link
Member

m1r0 commented Mar 27, 2025

Forgot to mention - could you please bump the minimum WP version so we don't forget?

@renatho
Copy link
Contributor Author

renatho commented Mar 27, 2025

Thank you for the review, @m1r0!

For the warnings, I honestly didn't go deeper. These Sass warnings are probably related to dependency versions on Gutenberg. I tried to build the project there to see if they have the warning. But they are supressing some parts of the output. But again, I didn't go deeper and it would be nice to check later.

For the lesson layouts, it's mapped in the issues we're working on: #7778.

Forgot to mention - could you please bump the minimum WP version so we don't forget?

Maybe it would be better to wait in case we decide to release earlier or if something happens with the WordPress release, delaying it?

I'm merging this, but I could raise another PR to bump the version if you still think it's worth it.

@renatho renatho merged commit 050c847 into trunk Mar 27, 2025
22 checks passed
@renatho renatho deleted the update/wordpress-dependencies branch March 27, 2025 18:14
@m1r0
Copy link
Member

m1r0 commented Mar 27, 2025

Maybe it would be better to wait in case we decide to release earlier or if something happens with the WordPress release, delaying it?

I'm not sure if I understand the logic here. I thought if we use the wp-6.7 version in package.json, we should also declare that version as the minimum supported version for the plugin. Isn't that the correct approach? About the WordPress release part, do you mean waiting for the 6.8 release and declaring that as the minimum version?

@renatho
Copy link
Contributor Author

renatho commented Mar 28, 2025

I'm not sure if I understand the logic here. I thought if we use the wp-6.7 version in package.json, we should also declare that version as the minimum supported version for the plugin. Isn't that the correct approach?

I think it doesn't have relation since it's mostly about dev scripts (build, lint, tests...). The only exception would be the styles that we're bundling in our codebase #7782. But this could cause unexpected issues with any combination of version (we installing the 6.7 dependencies and the user using WP 6.5, and vice-versa).

Does it make sense?

About the WordPress release part, do you mean waiting for the 6.8 release and declaring that as the minimum version?

I meant just bump the version when 6.8 is released, regardless of this PR. It would be:

Requires at least: 6.6
Tested up to: 6.8

@m1r0
Copy link
Member

m1r0 commented Apr 1, 2025

Ok, makes sense now. Thanks for explaining!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

4 participants