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

Add react-native-linear-gradient lib #13060

Merged
merged 2 commits into from Feb 6, 2020

Conversation

lukewalczak
Copy link
Contributor

@lukewalczak lukewalczak commented Dec 9, 2019

Fixes #

Ref to gb-mobile: wordpress-mobile/gutenberg-mobile#1616
Ref to gutenberg: WordPress/gutenberg#18823

This PR introduces 1st iteration of mobile Button block component.

To achieve gradient background color on mobile react-native-linear-gradient comes in handy.

The first iteration includes:

  • Rendering Button with:
    • gradient background color ✅
    • custom background color chosen from picker ✅
    • custom text color chosen from picker ✅
  • Options to change border radius, add link, open url in new tab ✅
  • Expanding and shrinking RichText
  • Notification for missing color settings functionality ✅

To test:

-> Gradient

  • open draft post on web
  • add Button
  • choose the gradient background color
  • save post
  • open draft post on mobile
  • check if a gradient is correct

Screenshot / gifs:

IOS

regular linear gradient
Screenshot 2019-12-02 at 11 05 07 Screenshot 2019-12-02 at 10 58 11

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.

@lukewalczak lukewalczak added [Deprecated] Gutenberg compatibility Deprecated label, use "Gutenberg" instead. Gutenberg Editing and display of Gutenberg blocks. labels Dec 9, 2019
@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Dec 9, 2019

You can trigger an installable build for these changes by visiting CircleCI here.

@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Dec 9, 2019

Warnings
⚠️ This PR is tagged with 'DO NOT MERGE'.
⚠️ PR is not assigned to a milestone.

Generated by 🚫 dangerJS

@iamthomasbishop
Copy link

@lukewalczak this is coming along very nicely. Here are a few notes I made as feedback during testing (on WPiOS via the build mentioned above):

  • Color settings informational bottom sheet: We can remove the single-quote marks from the title. Right now it says 'Color Settings' aren't yet supported... – it can just be Color settings aren't yet supported.... We can probably also remove the description below that. (screenshot)
  • The left/right padding on the button seems a bit too much. What are the paddings set at? IIRC the figma component has 16px.
  • Link rel value placeholder should be None instead of Add URL (screenshot)
  • Tapping on the link rel input is causing weird flickering/freezing issues (video for example)

Let me know if you have any questions!

@lukewalczak
Copy link
Contributor Author

Thanks for a feedback @iamthomasbishop!

Color settings informational bottom sheet: We can remove the single-quote marks from the title. Right now it says 'Color Settings' aren't yet supported... – it can just be Color settings aren't yet supported.... We can probably also remove the description below that.

Ok, I'll adjust it according to your suggestion.

The left/right padding on the button seems a bit too much. What are the paddings set at? IIRC the figma component has 16px.

Actually, horizontal paddings have 16px, however RichText itself has also some space. Please look at the screenshots:

paddings - one character | paddings - more characters |
paddings - empty rich text | no paddings
--- | --- | --- | ---
Screenshot 2019-12-17 at 12 53 58 | Screenshot 2019-12-17 at 12 54 54 | Screenshot 2019-12-17 at 13 01 17 | Screenshot 2019-12-17 at 12 54 15

Link rel value placeholder should be None instead of Add URL

Will change it!

Flickering / freezing issues

Working over that, however, as I mentioned on slack we will not face these issues once we choose an approach where only when user tap directly the RichText within the Button it will be focused (still tapping outside the Button area will select that block).
That approach is bug-less and following the web approach. What's more it can be confusing for a user that keyboard is opening on taping the Button block once user wanted eg change the border radius.

Let me know what do you think!

@iamthomasbishop
Copy link

Actually, horizontal paddings have 16px, however RichText itself has also some space. Please look at the screenshots:

Ahhh, that makes sense (that extra spacing issue has been creeping up in a bunch of places 😞 ). I think we're solving it elsewhere, so no need to change here.

Your notes regarding tapping directly on RichText vs the block boundary makes sense. Although I think most users will expect that an initial on the button itself will focus it, I think once they're familiar of the concept of blocks and can see where the boundaries are, it'll be more obvious. I'll keep an eye on this during upcoming usability tests.

@pinarol
Copy link
Contributor

pinarol commented Feb 5, 2020

LGTM! Tested with the steps I mentioned in the gutenberg PR.

But, since the release branch is cut, we should merge into gutenberg/after_1.22.0 instead of develop.

(Another option is waiting until the release branch is merged into develop again which will happen next Monday)

@lukewalczak lukewalczak changed the base branch from develop to gutenberg/after_1.22.0 February 6, 2020 14:06
Copy link
Contributor

@pinarol pinarol left a comment

Choose a reason for hiding this comment

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

LGTM!

@lukewalczak lukewalczak merged commit ea44373 into gutenberg/after_1.22.0 Feb 6, 2020
@lukewalczak lukewalczak deleted the feat/linear-gradient branch February 6, 2020 17:57
@hypest hypest mentioned this pull request Feb 11, 2020
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Deprecated] Gutenberg compatibility Deprecated label, use "Gutenberg" instead. Gutenberg Editing and display of Gutenberg blocks. [Status] DO NOT MERGE
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants