-
Notifications
You must be signed in to change notification settings - Fork 57
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
Unsupported block fallback #2063
Conversation
532caa2
to
e102ae2
Compare
This is used to display a web view rendering the unsuported block. At first we might show the full post loading gutenberg-web, but ideally we would show just the requested block.
To replace a single block with the given blocks in HTML code.
f4aa159
to
d49be0c
Compare
aa61283
to
059425a
Compare
Hey @etoledom, I took some time last night to review progress via the test build (only on iOS) and jotted down some notes. I tried to section things off into mobile-editor specific and web-editor specific. Let me know if you have any questions or concerns! Mobile editor
Web editor
|
Thanks for the feedback @iamthomasbishop ! While I work on them, I have a few questions:
Same color for dark mode?
Yes. This is what I was using before, but the user can close it by sweeping down and lose the content, so I decided to go with full screen. We can go with the modal presentation. This is how it looks:
Not sure what do you mean by this 🤔
I do see that bar on Tablets when the web view is presented full screen. |
Good question. What I meant is we should use the main blue, which IIRC is
Isn't there a way to disable swipe-to-dismiss in these types of modals? If so, let's disable that.
What I mean is on the web (on a bigger screen), if you go to the top-level
This part is fine, I just think when the block toolbar is active (when a block is selected), I'd like it to be top-aligned instead of floating (if possible — this isn't a dealbreaker).
Ahh, that's a good point. Can ignore this point then, if that element isn't shown on that view. Although for any kind of nested block, that would be handy to have (because navigating nesting on the current web UI without it is nearly impossible). |
Just found a way! 🎉 |
So I guess it is a problem to be addressed on the Web side. I know InnerBlocks navigation is a work in progress on web, so we might have some improvements for free there. Most of the time we will be showing the smaller screen size that doesn't show this navigation option. On iPad, using the modal presentation will solve a big problem (to hide the Page settings when no block is selected). So probably we won't have this navigation option even on iPads. I'd like to go with the modal presentation on iPad for this reason if that's fine.
I tried that out before, since it looks promising. The problem is with the block toolbar imbedded inside the top toolbar, it becomes quite difficult to remove all other elements from the top toolbar. With he current approach we remove the whole top bar by setting it's height to 0 (since the block toolbar is still a child, we can't just use But again, using modal view, the block bar will be pinned at the top anyway as we want it to be! 🎉 Soon will push an update. |
@etoledom Thanks for prepping another build to test on WPiOS! I had a chance to test and most everything is looking solid — just a few bits of feedback: Mobile editor
Web editor
|
@iamthomasbishop - I'm having problems with this. Ideally we won't want to show this dialog if there are no changes, but it's getting difficult to know if there are changes. The HTML returned by the parser is (usually) different to the one given even if there are no changes made by the user. And the flag We can always show the dialog even if the user did nothing, or we can just not show it (at least for this first iteration until we figure out a better way). Maybe we can change the |
I hadn't thought of bypassing the running of the mobile app and testing directly with a site running in Docker @etoledom! (I've done that once locally and used the instructions in the Gutenberg repo).
That would be done by using the tip of Gutenberg's master branch, correct?
I agree, we would ideally do it on a browser sized for mobile to make sure the experience is similiar to what our web view gets.
Yes, I think that would be perfect for an initial iteration. Then a second iteration could include the visual snapshot tests that would allow us to catch new UI elements or other changes in the editor UI. In summary, what I understand is the current plan is to:
This covers self-hosted WordPress, but I'm not sure if we need to cover other cases like .com – or if they're they'll be identical. Also, I considered whether we can further "short-circuit" point (2) above to check for the CSS classes directly in the code-base without spinning up an entire WordPress instance in Docker, but that wouldn't be a reliable indicator of whether they're present in the actual running webpage. |
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.
Tests on iOS build #29262
Overall, things are working very well – except for one authentication issue. I found that under certain conditions I'm asked to log in again upon opening the unsupported block editor. This seems to occur only when using a site that is not hosted on a wordpress.com
domain (note, Atomic sites are also affected).
The authentication issue
On opening the unsupported block editor using any non WordPress.com site (for example Atomic/Jetpack/self-hosted), I'm asked to log in again (reproduced consistently). I think the trick to reproducing this bug is to make sure you don't open the unsupported block editor from a .com site first – just go straight to a post on one of these affected platforms after doing a fresh install of the app.
It appears that opening the editor on a .com site caches authentication credentials which are then used when I then open a different type of site.
Atomic Site | Self-hosted with Jetpack |
---|---|
Scenarios tested
- WPCom
- WPCom Atomic
- Self Hosted with Jetpack
- Self Hosted without Jetpack
Scenarios not tested
- I haven't yet tested different WordPress versions we cover: v5.0, v5.1, v5.2, v5.3
- Since authentication issues appear to be domain related, I also want to test WordPress.com sites that are mapped to a custom domain (non-
*.wordpress.com
).
What's working great 🎉
- The editing experience feels close to being ready for launch
- I could edit blocks without a hitch (I used Audio and Twitter blocks for tests)
- Love how dark mode works seamlessly in the web view
- The progress indicator looks great
What needs attention ⚠️
- There's an authentication issue I'll explain below on non .COM sites
- The keyboard Done button (just above the keyboard) has really low contrast (white text on a grey background) – not sure if we can apply a tint to it specifically
Outstanding issues
A couple of things I think we haven't made a decision on:
- Are we going to leave as-is the title for the action button in the bottom sheet? It's "Edit block in web browser" which could work, but I found a little confusing from a user's point of view
- We don't have pretty names for some blocks, but that appears to be outside our control – can we ship in the current state?
A minor detail I noticed:
- When deleting an unsupported block, the bottom sheet says "Remove Unsupported" – this is actually not part of this project as such but may be worth adjusting.
See overview of tests performed
WordPress.com
- I was able to edit an unsupported block successfully ✅.
Atomic Site
Site: https://pschrottkyatomicsite1.blog/
- On opening the web view using an Atomic site, I'm asked to log in again (reproduced 3/3).
- Once logged in I tested with an Audio block and I was able to edit the block successfully ✅.
Self Hosted with Jetpack
Site: https://agreed-green.jurassic.ninja
- Here I had the same problem where the web view would ask me to login. The difference here is that trying with a .com site first and then switching to this Jurassic Ninja site didn't fix the issue. Maybe cookies aren't being persisted because they're different domains (i.e. wordpress.com vs jurassic.ninja).
- I was able to edit an unsupported block successfully ✅.
Self Hosted without Jetpack
Site: https://nuclear-smelt.jurassic.ninja
- The same login issue is present when opening the web view, but what's telling here is that if I had previously logged in to the previous site (https://agreed-green.jurassic.ninja), I didn't have the issue. This reaffirms the idea that authentication is being cached on a per domain basis.
- Editing a block worked fine ✅.
cc @etoledom
@marecar3 thanks for the heads-up! I tested the Android build today while keeping this issue in mind. Tests on Android build #53473Similar to the iOS tests above, the experience overall was great – the known authentication issue is the only major issue I came across. Below I also cover some minor details I came across which I haven't seen mentioned before. Scenarios testedWPCom, WPCom Atomic, Self Hosted with Jetpack, Self Hosted without Jetpack Scenarios not tested
What's working great 🎉
What needs attention
|
# Conflicts: # ios/Podfile.lock
@guarani that's interesting, good catch! I'm not sure where this is coming from, but I don't think we've explicitly changed anything WRT the progress bar (@etoledom @marecar3 is that correct?). Assuming it's not something we changed, we prob don't need to worry about it too much as part of this particular work, but we could create a ticket/proposal to align the platforms. |
Here’s what I have found so far:
I talked with @aerych (since he helped setting this up) and we agreed on preparing a PR to check this out. This would be pretty much to add a check if the site is also private here: And to try to figure out why private atomic doesn’t work, even when we get the cookie. I think it would be good to know if it’s expected for this endpoint to return 500 for public atomic sites too. @khaykov also might help us since (afaiu) he implemented these Android side of it. cc @guarani |
<!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd"> | ||
<plist version="1.0"> | ||
<dict> | ||
<key>PreviewsEnabled</key> |
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'm not sure what PreviewsEnabled
does here in workspace settings cc @etoledom
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.
Verified not showing up with the DEV flag = false. Excited to keep using this as we fix the final issues and get it ready to ship, such a cool feature! 🎉
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.
Tests
On both iOS and Android
- ✅ Option for editing unsupported blocks appears on development builds
Runyarn start:reset
in one window andyarn ios
in another - ✅ Option for editing unsupported blocks does not appears on release builds
This PR implements a message to the gutenberg bridge when the user taps to open an unsupported block on a web view.
Gutenberg
side PR: WordPress/gutenberg#21150WPiOS
side PR: wordpress-mobile/WordPress-iOS#13967WPAndroid
side PR: wordpress-mobile/WordPress-Android#11919To test:
(?)
button on the unsupported block.Edit block in web browser
option.Continue
to save the changes.We need to test these steps in:
Note: For WP v5.0 and 5.1, the interface is not completely cleaned up. You might find extra not-needed elements. I opted to not add extra complexity to fix these cases.