-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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/integrate release 1.31.0 #12258
Conversation
…d-from-monorepo [Gutenberg] Enable build from monorepo
You can trigger optional UI/connected tests for these changes by visiting CircleCI here. |
You can test the changes on this Pull Request by downloading the APK here. |
@hypest asked me to test the PR on Windows. I cloned a fresh copy, created the submodule and ran Build logs for when
Build logs for when
I think this ^ is pretty much expected. |
Edit: On a subsequent run the build worked as expected as mentioned here. On my MacOS machine where I do WPAndroid releases, I tried to run
|
@mchowning was able to run the The modified files are still an issue though and it'd be really good to fix those as I keep having to discard the changes in each (release) build. |
I've been testing the building from source with metro running from GalleryGallery-1
Gallery-2
Gallery-3 The enter button does not work in captions. I know this is already a known issue, but it used to be possible to insert newlines by "splitting" text, which is no longer possible. Is this intentional?
Gallery-4
Attachment page seems to be broken on both web and mobile for self-hosted sites - the links do not work in the preview. ColumnsColumns - 1
Columns - 2
Columns - 3
MediaTextMediaText-1
MediaText-2
MediaText-3
Video thumbnail not working on Android (displayed as black box) Also bug: preview has to be clicked twice to work. MediaText-4 CoverCover-1
Cover-2
Cover gradients did not work for a self-hosted site (is this a known issue?) MiscShortcode-1
MultipleUpload-1
Great work on this one, very glad to see this coming to fruition! |
Thanks for trying out the PR @oguzkocer !
Interesting, looks like a subcommand is trying to update some submodules and fails because of git, probably not finding git in the path (of a subshell). The "-q" parameter in that command looks weird as it's not something we usually have in the manual git command we instruct people to perform, plus AFAIK we never do the submodule updates via a node/npm command. While at it let me ask, do you see the
When in Edit: Just leaving a note here that trying to pull the submodules on this PR on my Windows10 setup resulted in a |
Thanks for the review @mkevins ! 🙇
This was intentional. Having it break out a new block below the "captioned" block is matching the behavior on web. Likewise, the fact that if the cursor is in the middle of a caption pressing Enter does nothing, is also a behavior we are inheriting from web. That seems like a bug to me. (comment) I'm noticing that with the gallery block the enter key is working as described ^^ for the gallery's caption, but the enter key does nothing in the "caption" for each individual image. On the web, pressing the enter key in an image's caption within a gallery block creates a new line, so the fact that we're not doing this feels like a bug on our side. If you agree, should we open an issue for this @mkevins ? None of ^this^ caption behavior is a regression from the previous release (the caption changes were merged in the last release), so I don't think any of these caption issues are release blockers.
I see this as well. Same issues are occurring with the previous mobile release, so this does not appear to be a regression. I'm not seeing any current issue tracking this. Would you mind adding one @mkevins ?
The black box issue is a longstanding problem, and should not be a blocker for the release. I wasn't able to recreate the issue with having to click the preview twice to work. |
Performing a round of sanity tests using this APK DarkMode-1
I did come across this again while testing wordpress-mobile/gutenberg-mobile#2209 Group-1
Spacer-1
Buttons-1
Buttons-2
Buttons-3Button-6 |
Hey @mkevins, just to confirm did you have a custom theme installed on this site? I was able to reproduce but only by:
Assuming these steps are accurate, @pinarol and I didn't feel like this is a blocker although I do think its a regression we should address. I opened up this issue to track it. You can also confirm by doing a fresh install and going to a self-hosted site that has a theme like Twenty-Twenty active. |
I am running the commands from a git shell, so I was pretty sure the path wasn't the problem, but just to be on the safe side, I added the necessary paths to my environment variables and it still failed with the same error. I also updated my Git with no luck.
I am able to run the command manually without any issues. In my previous attempts to resolve issues with Gutenberg builds on Windows, I found that a lot of the time the build error wasn't the actual issue and I had to do some manual tinkering to get it to show itself, so when I saw that error I thought this would be the case. (your experience with the longer path name is one of those weird Windows issues I am afraid)
I figured this would be the case and @mchowning brought up the same thing. I'll give this a try when I update my main repo. I am working on a project right now and I don't want to mess with my setup, so I am leaving my Windows machine for project work and doing everything in mac for now. I'll let you know if I have any issues with this.
As much as I appreciate you giving Windows some love 🤗, unless there is an urgent reason we want this setup to work in Windows, I wouldn't spend too much time on it. (if it was working with yarn, I wouldn't say that, but since it's already broken 😿 ) My next project will be to move the whole thing to Bintray or Github packages so we don't have to deal with the Gutenberg builds on the WPAndroid side. (unless it's being built from source of course) Once that's in place, I am sure Windows will go back to being a happy-land, I'll make sure of it 😛 |
I tracked down the issue for #12258 (comment) It ended up being an issue with WellSQL. Not sure what the cause of the change amounted to be but fetching zero results now returns an empty list instead of null. Made the adjustment here to catch that. wordpress-mobile/WordPress-FluxC-Android#1620 cc @mkevins |
@oguzkocer Ah, I was under the impression that you were actually using your Windows setup for the normal app releases tasks, and I wanted to make sure we are not breaking your flow there! Sorry I misunderstood 😬. Windows support is not a priority otherwise and we can leave it at that. Thanks again for trying this on your end! |
Oh no, WPAndroid has been broken for me for the past month or so 😂 Thanks for checking in anyway! |
Hi @mchowning 👋 , thanks for confirming some of the the issues I encountered. I agree these are not blockers for this release, and mostly I left them in this long comment to document. I can open the issue for the gallery link to issue to track that one. |
I first encountered this via a jurassic ninja site (not connected via Jetpack), using the default Twenty-Twenty theme (I didn't modify the theme). I just attempted to recreate the issue, and using a gradient as a cover background, I was not able to reproduce it. Then, I tried creating a different post, this time with gradients as an overlay, and the gradients did not render (the image still did). Curiously, when I reopened the post I had created earlier (with the gradient backgrounds), the gradients were gone. 🤔 |
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.
Looks good to me!
I only left a question for Ceyhun and Tug but I don't consider it a blocking one.
Merging Gutenberg 1.31.0 Release. 🎉
For changes that need to be tested, take a look at the related gutenberg-mobile PR: wordpress-mobile/gutenberg-mobile#2423
This is the first release with the monorepo changes, so please take extra care to test the WPAndroid build process, and ensure we can build:
wp.BUILD_GUTENBERG_FROM_SOURCE
set to either true and falsePR submission checklist:
RELEASE-NOTES.txt
if necessary.