-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Gutenberg: update npm packages to latest versions #28000
Conversation
@@ -1,4 +1,4 @@ | |||
@import '../../../node_modules/@wordpress/components/build-style/style'; | |||
// @import '../../../node_modules/@wordpress/components/build-style/style'; |
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.
cc @mmtr since you've worked on this. If I leave the style include here I'm running into:
{
"status": 1,
"file": ".../wp-calypso/node_modules/@wordpress/components/build-style/style.css",
"line": 1615,
"column": 27,
"message": "Function shade is missing argument $percent.",
"formatted": "Error: Function shade is missing argument $percent.\n on line 1 of client/devdocs/gutenberg-components/style.scss\n from line 7 of assets/stylesheets/sections/devdocs.scss\n>> background: #0085ba shade(15%); }\n\n --------------------------^\n"
}
The mentioned shade
function has the argument percent
passed at that line if you check the built file. Any ideas what might be causing 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.
I'd say that the shade
function shouldn't be in the generated CSS file given that it's not a CSS function AFAIK but I can see it on the original SCSS file (added on WordPress/gutenberg#7621). I'm sure that function should be converted to something else during the CSS build process.
I see the shade
function is also used in other components (i.e. Button
), but in all of them the function is an argument of the color
function, so maybe the issue is on Gutenberg and it can be solved if we replace background: theme(primary) shade(15%);
with background: color(theme(primary) shade(15%));
on the original SCSS file.
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.
Confirmed that will solve the issue. Gutenberg uses postcss-color-function
which provides the color
interface and shade
as one of their color adjusters.
I can open a PR fixing that as soon as I got access to the Gutenberg repo :)
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.
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 already asked for that in p1540284879000100-slack-gutenberg. I think only @mtias can add me.
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, him and @pento :)
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.
@mmtr if you don't want to wait feel free to start a PR from your fork too, but we'll definitely want to provide you with access for future fixes.
Also, it's curious that the same import wasn't causing issues for editor shell in Calypso:
https://github.com/Automattic/wp-calypso/blob/master/assets/stylesheets/sections/gutenberg-editor.scss#L19
That's why I suspected it had something to do with differences in DevDocs setup at first.
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.
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 Miguel!
We are close to the real API freeze, one week left :D |
I see two errors I haven't seen yet: First is in: Second in: Other than that, though, I've smoke tested Guten, and it's working as well as before. :) |
Thanks for testing @Copons! The first one is already present in master, since the importing of Jetpack blocks (#27864), and it's kind of expected (translations issue). I haven't noticed the second one before (will have to re-test), but it sounds like the problem with API path mapping. If there is some specific action that triggers it, knowing that would be helpful. |
Oh, apparently somebody forgot to pull master, eh! 😤
I thought it popped up upon first load, but in fact it is triggered by the sidebar. |
@vindl renovate will automatically generate PRs for updates to the WordPress monorepo (see #27592). In the future, could we please piggyback on those PRs instead of opening separate ones? It keeps the shrinkwrap normalized, gives us automatic release notes, and folks know where to look for module updates and makes is easier to report on the number of PRs related to managing dependencies. Feel free to continue on here and if you need any help, we're glad to assist. Thanks much! |
On the CSS issue, I noticed that we're pulling in the built CSS from Gutenberg. wc-admin uses a slightly different tact, pulling in the SCSS and processing it with postcss to theme out some colors. Is that something we'd be interested in doing for Calypso? If it is, is there an issue tracking it? Probably a bit related to #26910 |
@blowery sure! I wasn't aware of all those benefits that renovate brings. If you want I can close this one too and continue there. My main motivation so far was to merge these updates as soon as possible, because they might affect all the other areas of our work. Is there a way for me to trigger those renovate PRs manually just for WordPress monorepo? |
On hold until new version of |
I believe we discussed this briefly at one point, and for now we are not planning to allow theming for Calypso integration (cc @gwwar @Copons please correct me if I got this wrong). |
@vindl it's fine here! I just wanted to make sure you were aware of the renovate PRs. :) |
@Copons the problem here seems to be that |
bd8016b
to
3444b05
Compare
Added some additional fixes to make the editor boot.
3444b05
to
c7aa2ba
Compare
I updated all of the related packages again to the newest versions and introduced a couple of fixes that are needed for editor to boot (will point those out in inline comments). There is one new error regarding the rest route that probably requires a new mapping in middleware, and I'm planning to tackle that in a separate PR in order not to drag this one further. Otherwise, this is ready for another round of reviews and testing. |
@@ -38,7 +38,6 @@ function registerDataPlugins( userId ) { | |||
const storageKey = 'WP_DATA_USER_' + userId; | |||
|
|||
use( plugins.persistence, { storageKey: storageKey } ); | |||
use( plugins.asyncGenerator ); |
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 has been removed from the latest version of @wordpress/data
package: https://github.com/WordPress/gutenberg/blob/master/packages/data/CHANGELOG.md#300-2018-10-29
@@ -58,9 +57,6 @@ function Header( { | |||
isToggled={ isEditorSidebarOpened } | |||
aria-expanded={ isEditorSidebarOpened } | |||
> | |||
<DotTip id="core/editor.settings"> |
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 was causing the editor to break, so I'm removing it since this is the only usage of DotTip
in edit post, and we don't want to use the guided tour anyway. I'm suspecting its related to some specific error with @wordpress/nux
package and not to our integration.
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.
Smoke tested and couldn't find any differences with the current wpcalypso version, except the uncaught error I've already reported a few days ago, but we can handle that in a follow up.
In other words, LGTM!
Changes proposed in this Pull Request
Updates Gutenberg npm packages to latest versions.
It seems like this update will not require as much fixes as the ones before it. :)
Testing instructions
On hold until new version of components package, that contains WordPress/gutenberg#10943 fix, is published.