-
Notifications
You must be signed in to change notification settings - Fork 209
Update npm dependencies #7695
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
Update npm dependencies #7695
Conversation
427e4f8 to
41fe999
Compare
41fe999 to
710033b
Compare
|
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. |
WordPress Dependencies ReportThe
This comment was automatically generated by the |
|
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. |
3e6bfe9 to
6ca3cf5
Compare
|
Test the previous changes of this PR with WordPress Playground. |
|
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", |
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.
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.
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.
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 } /> |
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.
Make sure the key will be always available since url is not always available.
|
Test the previous changes of this PR with WordPress Playground. |
@wordpress)|
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. |
| * @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. |
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 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.
|
Test the previous changes of this PR with WordPress Playground. |
c4cea5c to
373c916
Compare
|
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. |
…press-dependencies
4760307 to
825da2f
Compare
|
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.
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.

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

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.

Again, thank you for working on this! 🙇
|
Forgot to mention - could you please bump the minimum WP version so we don't forget? |
|
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.
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. |
I'm not sure if I understand the logic here. I thought if we use the |
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?
I meant just bump the version when 6.8 is released, regardless of this PR. It would be: Requires at least: 6.6 |
|
Ok, makes sense now. Thanks for explaining! |
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
Testing Instructions
Students menu (#6123)
Sensei Pro
Pre-Merge Checklist