Skip to content
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

Enable Button block in production #1866

Merged
merged 3 commits into from
Feb 14, 2020
Merged

Conversation

lukewalczak
Copy link
Contributor

@lukewalczak lukewalczak commented Feb 7, 2020

Unblocking a Button block in the production

Ref to gutenberg: WordPress/gutenberg#20093

To test:

https://github.com/wordpress-mobile/test-cases/blob/master/test-cases/gutenberg/button.md

PR submission checklist:

  • I have considered adding unit tests where possible.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@pinarol
Copy link
Contributor

pinarol commented Feb 7, 2020

I am copy/pasting test cases here to be checked after testing:

WPiOS:

  • Button block - Render custom text color - steps
  • Button block - Render custom background color - steps
  • Button block - Render gradient background color - steps
  • Button block - Check if selection / caret color matches font color - steps
  • Button block - Edit text styles - steps
  • Button block - Fallback to default colors in case theme colors are used - steps
  • Button block - New Buttons are created with the blue color - steps
  • Button block - Settings: Change Button border radius - steps
  • Button block - Settings: Open in new tab - steps
  • Button block - Settings: Link rel - steps
  • Button block - Settings: Link URL - steps
  • Button block - Settings: Remove link - steps
  • Button block - Settings: Synchronize with button options - steps
  • Button block - Link in the clipboard is automaticially added into the empty URL field - steps
  • Button block - Button max width is calculated OK inside inner blocks(iOS only) - steps

WPAndroid:

  • Button block - Render custom text color - steps
  • Button block - Render custom background color - steps
  • Button block - Render gradient background color - steps
  • Button block - Check if selection / caret color matches font color - steps
  • Button block - Edit text styles - steps
  • Button block - Fallback to default colors in case theme colors are used - steps
  • Button block - New Buttons are created with the blue color - steps
  • Button block - Settings: Change Button border radius - steps
  • Button block - Settings: Open in new tab - steps
  • Button block - Settings: Link rel - steps
  • Button block - Settings: Link URL - steps
  • Button block - Settings: Remove link - steps
  • Button block - Settings: Synchronize with button options - steps
  • Button block - Link in the clipboard is automaticially added into the empty URL field - steps

@pinarol pinarol requested review from dratwas and geriux February 7, 2020 18:15
@pinarol
Copy link
Contributor

pinarol commented Feb 7, 2020

@geriux -> WPAndroid
@dratwas -> WPiOS

Please use peril links in the parent PRs to install the app for testing. And check these items when done with testing. Keep in mind that you don't have to limit yourselves with those items, challenging the feature in different ways is always welcomed.

@iamthomasbishop
Copy link
Contributor

iamthomasbishop commented Feb 7, 2020

Had a chance to go through and test extensively, here are my notes. Note: Most issues were present across both platforms (platform-specific issues are labeled as such).

Blockers

  • I think the fallback color should be a neutral gray (#595959), not blue. This is esp noticeable when using a theme like Twenty-Twenty, which has a strong background color (screenshot)
  • Tapping delete key while button is empty should delete the block
  • When button has link, toolbar button should have “active” button style (example)
  • We should probably show the link button in the block toolbar while the block is selected. (Not only while richText is focused) (example)

Non-blockers

  • [Android-only] Left/right inner padding on buttons seems a bit larger on Android
  • [Android-only] Android text selection highlight is appropriate color, but handles aren’t the same color as the button text. Not a blocker, but would be nice to match (screenshot)
  • Button alignment settings are not honored, or shown in the block toolbar (screenshot)
  • Button placeholder text seems a bit too strong, can we use white with alpha 43%? (example)
  • Can we move the highlight outline to “outside” the button itself, so that the left side of the button fill is aligned left, not the highlight? (screenshot)
  • Button URL field on bottom sheets (link settings, block settings) should be changed to Button Link URL
  • For color settings, we can probably remove the section header and leave only the footer note. Also, now that I see the footer note in context, I think it should instead read: “Button color settings are coming soon.” (example)
  • We can remove the border-bottom on the Link rel cell (screenshot)

@lukewalczak
Copy link
Contributor Author

lukewalczak commented Feb 8, 2020

Thanks @iamthomasbishop for the valuable feedback! 👌

To avoid confusions I would like to ask for some details:

I think the fallback color should be a neutral gray (#595959), not blue. This is esp noticeable when using a theme like Twenty-Twenty, which has a strong background color (screenshot)

Unfortunately, a screenshot is not loading and tbh I don't know what color you meant, because please note that Button bg color can be set in three different cases:

  1. blue (#2271b1) - creating new Button block on mobile
  2. gray (#595959) - fallback color when we cannot resolve color set on the web
  3. custom color - color we are able to resolve

[Android-only] Android text selection highlight is appropriate color, but handles aren’t the same color as the button text. Not a blocker, but would be nice to match

I think it can be related to the Android API version, but would like to get more insights from @chipsnyder who was working on that (is there any obstacle to support it?). On API level 27 handles have correct bg color, for the reference:

API 27 API 29
Screenshot 2020-02-08 at 01 03 25 Screenshot 2020-02-08 at 01 00 28

Button alignment settings are not honored, or shown in the block toolbar

Didn't see that in the requirements, but I see no reason why don't support it ✌️ .

@iamthomasbishop
Copy link
Contributor

To avoid confusions I would like to ask for some details:

Sorry, I should have been clearer in my feedback. I meant to say that the background color should be gray both when the user creates a new button block on mobile and when there is a fallback.

The reason for this is — and correct me if I’m wrong — is that when a user creates a new button on mobile, it is setting the background to the theme-default color. In other words, creating a new button on mobile results (on the front-end) in a button with the theme’s default style. For example, if I’m using the Twenty-Twenty theme and I create a button on mobile, it’s going to be a pink squared-off button. Because we don’t have access to that style information, we should keep the background color neutral.

On API level 27 handles have correct bg color, for the reference:

Good to know. 👍

Didn't see that in the requirements, but I see no reason why don't support it ✌️ .

I’m not sure if it was in the initial requirements, so it could be something to iterate on, but I just wanted to document it 😀

@lukewalczak
Copy link
Contributor Author

lukewalczak commented Feb 10, 2020

UPDATES - RESOLVE ALL BLOCKERS:

  1. Set bg color to gray (#595959), color displayed on preview comes from theme:

  1. delete key is deleting the block when a button is empty:

  1. Toolbar button has “active” button style when contains link:

  1. The link button is presented in the block toolbar while the block is selected:

Screenshot 2020-02-10 at 10 18 48

@dratwas
Copy link
Contributor

dratwas commented Feb 10, 2020

Tested the peril build in WP-iOS and every point seems to work 🎉 Also tried changing the orientation etc

@geriux
Copy link
Contributor

geriux commented Feb 10, 2020

Tested on Android using wordpress-mobile/WordPress-Android#11256 (comment)

By following these test cases #1866 (comment)

All working great ✅, orientation changes, using it within the Group block.. Couldn't break it =P great work! 👏

@geriux
Copy link
Contributor

geriux commented Feb 10, 2020

Couldn't test the latest changes though since the latest build doesn't include them and those were marked as blockers by @iamthomasbishop

@lukewalczak lukewalczak added the [Status] DO NOT MERGE Do not merge this PR label Feb 10, 2020
@chipsnyder
Copy link
Contributor

I think it can be related to the Android API version, but would like to get more insights from @chipsnyder who was working on that (is there any obstacle to support it?). On API level 27 handles have correct bg color, for the reference:

This is correct. The handles should be working in API 27. Although in API 28, there isn't a supported mechanism for it. Then in API 29 we would be able to support it once we bump Aztec to support API 29.

@lukewalczak
Copy link
Contributor Author

UPDATES - RESOLVE NON-BLOCKERS

  1. [Android-only] Left/right inner padding on buttons seems a bit larger on Android

That's true and the problem exists in RichText since the inner text is overlapping the paddings. Notice that once RichText is filled with content it looks properly.

  1. [Android-only] Android text selection highlight is appropriate color, but handles aren’t the same color as the button text. Not a blocker, but would be nice to match

Please check the @chipsnyder's answer above.

  1. Button alignment settings are not honored, or shown in the block toolbar (screenshot)

Via @pinarol: I see this as another iteration after shipping this version>

  1. Button placeholder text seems a bit too strong, can we use white with alpha 43%? (example)

I've updated the color (43% alpha of white)

light theme dark theme
Screenshot 2020-02-10 at 10 43 33 Screenshot 2020-02-10 at 10 43 07
  1. Can we move the highlight outline to “outside” the button itself, so that the left side of the button fill is aligned left, not the highlight? (screenshot)

I hope I understood it correctly and it looks properly right now:

default selected
Screenshot 2020-02-10 at 14 40 06 Screenshot 2020-02-10 at 14 43 36
  1. Button URL field on bottom sheets (link settings, block settings) should be changed to Button Link URL

Updated!

link settings button options
Screenshot 2020-02-10 at 14 44 45 Screenshot 2020-02-10 at 14 44 49
  1. For color settings, we can probably remove the section header and leave only the footer note. Also, now that I see the footer note in context, I think it should instead read: “Button color settings are coming soon.” (example)

Done 🎉 ! Please check the screenshot above on the right side.

  1. We can remove the border-bottom on the Link rel cell (screenshot)

Done 🎉! Also please take a look at the same screenshot!

@pinarol
Copy link
Contributor

pinarol commented Feb 10, 2020

Button alignment settings are not honored, or shown in the block toolbar (screenshot)

This is highly related with Buttons block work, it'd be better if we handle that as a part of that work. And we shouldn't delay it much, because the standalone Button block is kinda deprecated and it should be used inside Buttons block only.

@lukewalczak
Copy link
Contributor Author

lukewalczak commented Feb 10, 2020

NEW TEST CASES

WPiOS:

  • Button block - New Button is created with the gray color - steps
  • Button block - The newly created button has set background color to the theme-default color in preview - steps
  • Button block - Tapping delete key removes block when Button is empty - steps
  • Button block - Toolbar link button is active when Button has link - steps

WPAndroid:

  • Button block - New Button is created with the gray color - steps
  • Button block - The newly created button has set background color to the theme-default color in preview - steps
  • Button block - Tapping delete key removes block when Button is empty - steps
  • Button block - Toolbar link button is active when Button has link - steps

@dratwas
Copy link
Contributor

dratwas commented Feb 10, 2020

Tested the new build with new test cases and everything works great, however, I found an issue related to the link from the clipboard. If i select text wit link in it i.e:
Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris http://loremipsum.com The whole text is passed to the URL field. The issue is not only Button related since the paragraph link does the same.

Steps to reproduce:

  1. copy the text above to the clipboard
  2. create button / paragraph with link
  3. open link settings
  4. The URL is wrong since the whole text is passed to it

@pinarol
Copy link
Contributor

pinarol commented Feb 10, 2020

Thanks for reporting that @dratwas. Right, it looks like a general problem about pasting links.
@lukewalczak Let's open an issue for that and add to Mobile Gutenberg board.

@geriux
Copy link
Contributor

geriux commented Feb 10, 2020

Tested again with the new cases and all working great! 🎉

✅ Button block - New Button is created with the gray color
✅ Button block - The newly created button has set background color to the theme-default color in preview
✅ Button block - Tapping delete key removes block when Button is empty
✅ Button block - Toolbar link button is active when Button has link

By the way regarding to this:

Tested the new build with new test cases and everything works great, however, I found an issue related to the link from the clipboard. If i select text wit link in it i.e:
Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris http://loremipsum.com The whole text is passed to the URL field. The issue is not only Button related since the paragraph link does the same.

Happens with any text even if they don't have any link in it. So you can add it in the issue as well @dratwas =)

Copy link
Contributor

@geriux geriux left a comment

Choose a reason for hiding this comment

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

Approving but let's wait for @iamthomasbishop's OK on the design changes =)

Great work 👏

@iamthomasbishop
Copy link
Contributor

Most everything is looking good, so I think we can ship the block now and iterate. I have one clarification regarding the selection outline, which isn't a blocker.

I'm still seeing the outline as "inside" the button, whereas I was hoping that it could be "outside" (similar to how box-shadow is applied, to the outside of the element). Here's a diagram to better explain the preferred styling:

The benefit of placing the outline outside would be that it should prevent the button from "jumping" on the canvas — it stays in place.


I also agree with @pinarol's comment regarding alignment. I had forgotten that the Buttons block was recently introduced and will "deprecate" the Button block:

This is highly related with Buttons block work, it'd be better if we handle that as a part of that work.

@lukewalczak
Copy link
Contributor Author

@iamthomasbishop thanks for an explanation! I think I've found the desired solution:

unselected selected
Screenshot 2020-02-10 at 22 31 32 Screenshot 2020-02-10 at 22 31 30

@iamthomasbishop
Copy link
Contributor

That's looking great, just tried out the newest test build! :shipit: !

@lukewalczak lukewalczak removed the [Status] DO NOT MERGE Do not merge this PR label Feb 12, 2020
@geriux
Copy link
Contributor

geriux commented Feb 13, 2020

LGTM! Tested again on Android with the latest changes. Great work!

@dratwas
Copy link
Contributor

dratwas commented Feb 14, 2020

Tested on iOS and everything works as expected :) Good job @lukewalczak 🎉

@lukewalczak lukewalczak force-pushed the callstack/unblock-button branch from 61a8d09 to e445fce Compare February 14, 2020 10:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants