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

[Mobile] Create cover block with only colour (from a default palette) without an image #23833

Merged
merged 16 commits into from
Jul 17, 2020

Conversation

antonis
Copy link
Member

@antonis antonis commented Jul 9, 2020

Fixes #23831 (parent wordpress-mobile/gutenberg-mobile#2275)

Related PRs:

Description

A palette with four colours (black, white and a couple of theme defaults) has been added bellow the Add Media Button. When the user taps on a colour indicator the block is created with the selected colour as background.
The latest design can be found here.

How has this been tested?

  • Add a cover block
  • Press the one colour from the palette
  • Verify that a new cover block with only colour is created

Screenshots

Empty cover block Android iOS
Simulator Screen Shot - iPhone 11 - 2020-07-14 at 17 34 57 android ios

Types of changes

Enhancement

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.

@antonis
Copy link
Member Author

antonis commented Jul 13, 2020

Thank you for the feedback @geriux . I agree with both your suggestions.
I pushed those changes along with some alignments with the suggested design (#23870 (comment))

@antonis antonis self-assigned this Jul 14, 2020
@antonis antonis added [Block] Cover Affects the Cover Block - used to display content laid over a background image [Type] Enhancement A suggestion for improvement. labels Jul 14, 2020
@antonis antonis marked this pull request as draft July 14, 2020 11:53
@antonis antonis force-pushed the rnmobile/23831_Cover_Only_color branch from 71fec99 to 0cbe0c1 Compare July 14, 2020 13:21
@antonis antonis marked this pull request as ready for review July 14, 2020 14:36
@antonis antonis requested a review from chipsnyder July 14, 2020 14:38
@antonis
Copy link
Member Author

antonis commented Jul 15, 2020

Thank you for the review @geriux . I agree with both your suggestions and pushed the changes.

@antonis antonis force-pushed the rnmobile/23831_Cover_Only_color branch from 0ec00f0 to 5812607 Compare July 15, 2020 07:42
@antonis antonis force-pushed the rnmobile/23831_Cover_Only_color branch from 5812607 to a152a4a Compare July 15, 2020 08:06
@antonis antonis added the Needs Design Feedback Needs general design feedback. label Jul 15, 2020
@antonis
Copy link
Member Author

antonis commented Jul 15, 2020

Hello @iamthomasbishop 👋 ,
This PR is a step before #23870 (missing only the custom color picker).
I would like some feedback on this or the go ahead to proceed to the next PR.
Thank you for your guidance

Android WP screenshot with keylines

@iamthomasbishop
Copy link

Looks great! The empty block at first seems a bit tall (probably just because we're used to seeing shorter block placeholders), but I think the benefit of having it at the same size as its default application height is worth it.

Unrelated side note for @geriux I'm thinking we should perform a compact notice event when a block is added to the canvas. I don't know if we want to add that as part of this work or via a separate ticket -- perhaps we should create a new one as an audit to document where/when we want to add these notices.

@chipsnyder
Copy link
Contributor

Codewise this looks good to me. I do have a question on the Default Colors. I see we have Black and White plus colors from the theme.

When I tried white I wasn't able to see my text because the text for my theme (twenty-twenty) was also white:

On the web they seem to use only the theme colors:

Then if theme colors aren't available it fallbacks to the default color set. Can we consider replacing white and adding one more theme color? Or Replacing both Black and White and default to the theme colors?

@antonis
Copy link
Member Author

antonis commented Jul 16, 2020

Hello @iamthomasbishop and @chipsnyder 👋 ,
thank you both for reviewing this and your precious feedback.

@iamthomasbishop Would it be ok to drop the black/white colors of the initial requirement and use just the theme colors in order to cover the case @chipsnyder reported above?

@iamthomasbishop
Copy link

Would it be ok to drop the black/white colors of the initial requirement and use just the theme colors in order to cover the case @chipsnyder reported above?

Yea, that is fine.

Copy link
Member

@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.

LGTM! Nice work @antonis! Tested it on both iOS and Android as well as the demo app.

@antonis antonis merged commit 44295ce into WordPress:master Jul 17, 2020
@github-actions github-actions bot added this to the Gutenberg 8.6 milestone Jul 17, 2020
@antonis
Copy link
Member Author

antonis commented Jul 17, 2020

Hello @iamthomasbishop 👋 ,

The last two PRs of this feature are not ready yet. Specifically:

Should we ship this feature up to this point in Monday's release or revert it to ship it as a whole in the next release?

cc @pinarol

@iamthomasbishop
Copy link

Should we ship this feature up to this point in Monday's release

I'd be okay with shipping initially without those two things (custom color and the additional add-media button), assuming we can add these things in the next release or two.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Cover Affects the Cover Block - used to display content laid over a background image Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change) Needs Design Feedback Needs general design feedback. [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Mobile] Cover: Allow creating the block with only colour (without an image)
4 participants