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.

Sign up for GitHub

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

Jump to bottom

[RNMobile] Rounded image #24544

Merged
merged 17 commits into from
Sep 3, 2020
Merged

[RNMobile] Rounded image #24544

merged 17 commits into from
Sep 3, 2020

Conversation

lukewalczak
Copy link
Member

@lukewalczak lukewalczak commented Aug 13, 2020 •
edited
Loading

Fixes: #24489

Description

Added an option to set Rounded image.

How has this been tested?

  1. Open mobile app
  2. Open a post
  3. Add image block and choose the picture
  4. Open image settings
    5. Expect to see new options such as Default and Rounded
  5. Choose Rounded
    7. Expect that image is rounded
  6. Open a preview / open a post on web
    9. Expect to see rounded photo

Screenshots

  • ios - settings
light dark
Screenshot 2020-08-13 at 12 38 40 Screenshot 2020-08-13 at 12 38 24
  • ios - rounded photo
light dark
Screenshot 2020-08-13 at 12 38 36 Screenshot 2020-08-13 at 12 51 39
  • android - settings
light dark
Screenshot 2020-08-13 at 12 32 27 Screenshot 2020-08-13 at 12 35 10
  • android - rounded photo
light dark
Screenshot 2020-08-13 at 12 37 21 Screenshot 2020-08-13 at 12 35 20

Types of changes

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.

@lukewalczak lukewalczak added Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change) [Block] Image Affects the Image Block labels Aug 13, 2020
@lukewalczak lukewalczak self-assigned this Aug 13, 2020
@github-actions
Copy link

github-actions bot commented Aug 13, 2020 •
edited
Loading

Size Change: +4 B (0%)

Total Size: 1.17 MB

Filename Size Change
build/block-editor/index.js 128 kB +4 B (0%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.14 kB 0 B
build/annotations/index.js 3.67 kB 0 B
build/api-fetch/index.js 3.41 kB 0 B
build/autop/index.js 2.82 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/index.js 7.99 kB 0 B
build/block-directory/style-rtl.css 953 B 0 B
build/block-directory/style.css 952 B 0 B
build/block-editor/style-rtl.css 11.1 kB 0 B
build/block-editor/style.css 11.1 kB 0 B
build/block-library/editor-rtl.css 8.57 kB 0 B
build/block-library/editor.css 8.57 kB 0 B
build/block-library/index.js 137 kB 0 B
build/block-library/style-rtl.css 7.47 kB 0 B
build/block-library/style.css 7.47 kB 0 B
build/block-library/theme-rtl.css 741 B 0 B
build/block-library/theme.css 742 B 0 B
build/block-serialization-default-parser/index.js 1.88 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/blocks/index.js 47.7 kB 0 B
build/components/index.js 200 kB 0 B
build/components/style-rtl.css 15.5 kB 0 B
build/components/style.css 15.5 kB 0 B
build/compose/index.js 9.67 kB 0 B
build/core-data/index.js 12.3 kB 0 B
build/data-controls/index.js 1.29 kB 0 B
build/data/index.js 8.55 kB 0 B
build/date/index.js 5.38 kB 0 B
build/deprecated/index.js 772 B 0 B
build/dom-ready/index.js 568 B 0 B
build/dom/index.js 4.48 kB 0 B
build/edit-navigation/index.js 11.7 kB 0 B
build/edit-navigation/style-rtl.css 1.16 kB 0 B
build/edit-navigation/style.css 1.16 kB 0 B
build/edit-post/index.js 305 kB 0 B
build/edit-post/style-rtl.css 6.26 kB 0 B
build/edit-post/style.css 6.25 kB 0 B
build/edit-site/index.js 17.1 kB 0 B
build/edit-site/style-rtl.css 3.06 kB 0 B
build/edit-site/style.css 3.06 kB 0 B
build/edit-widgets/index.js 12 kB 0 B
build/edit-widgets/style-rtl.css 2.46 kB 0 B
build/edit-widgets/style.css 2.45 kB 0 B
build/editor/editor-styles-rtl.css 492 B 0 B
build/editor/editor-styles.css 493 B 0 B
build/editor/index.js 45.6 kB 0 B
build/editor/style-rtl.css 3.81 kB 0 B
build/editor/style.css 3.81 kB 0 B
build/element/index.js 4.65 kB 0 B
build/escape-html/index.js 733 B 0 B
build/format-library/index.js 7.71 kB 0 B
build/format-library/style-rtl.css 547 B 0 B
build/format-library/style.css 548 B 0 B
build/hooks/index.js 2.13 kB 0 B
build/html-entities/index.js 621 B 0 B
build/i18n/index.js 3.57 kB 0 B
build/is-shallow-equal/index.js 710 B 0 B
build/keyboard-shortcuts/index.js 2.52 kB 0 B
build/keycodes/index.js 1.94 kB 0 B
build/list-reusable-blocks/index.js 3.12 kB 0 B
build/list-reusable-blocks/style-rtl.css 476 B 0 B
build/list-reusable-blocks/style.css 476 B 0 B
build/media-utils/index.js 5.32 kB 0 B
build/notices/index.js 1.79 kB 0 B
build/nux/index.js 3.4 kB 0 B
build/nux/style-rtl.css 671 B 0 B
build/nux/style.css 668 B 0 B
build/plugins/index.js 2.56 kB 0 B
build/primitives/index.js 1.41 kB 0 B
build/priority-queue/index.js 789 B 0 B
build/redux-routine/index.js 2.85 kB 0 B
build/rich-text/index.js 13.9 kB 0 B
build/server-side-render/index.js 2.77 kB 0 B
build/shortcode/index.js 1.7 kB 0 B
build/token-list/index.js 1.27 kB 0 B
build/url/index.js 4.06 kB 0 B
build/viewport/index.js 1.85 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.17 kB 0 B

compressed-size-action

@lukewalczak lukewalczak force-pushed the rnmobile/rounded-image branch 2 times, most recently from 5ec90fd to 3eb16d0 Compare August 13, 2020 16:03
@lukewalczak
Copy link
Member Author

@iamthomasbishop Please look at the updated screenshots:

light dark
Screenshot 2020-08-14 at 10 30 21 Screenshot 2020-08-14 at 10 30 26

@iamthomasbishop
Copy link

@lukewalczak This looks great, spacing overall looks solid. I have only 2 tiny requests.

  1. Can we change the color of the selected item's label to our primary blue? I didn't want to add a super strong accent to the selection, but this would be a nice subtle refinement (https://cloudup.com/cyc5L1HvPRQ)

  2. I'm still not 100% happy with the bordering approach on the image preview. Ideally, to align with the 16 key line, the dimensions of the image background container should remain the same between selected/unselected states, with the selection border inset. So if we're using 100x64 for the container (same as the inserter item grids), here's what that would look like:

...and here's (roughly) what a row composition would look like when the first item is selected (left) and un-selected (right):

These are obviously super minor details and I'd honestly be fine with shipping this as-is, but while we're here we might as well get the nitpicky details right 😄

geriux
geriux reviewed Aug 27, 2020
View reviewed changes
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.

Hey @lukewalczak 👋

This is looking good! I left some minor comments. While testing on Android I found an overflow issue with the selected state border:

@lukewalczak
Copy link
Member Author

I'm still not 100% happy with the bordering approach on the image preview. Ideally, to align with the 16 key line, the dimensions of the image background container should remain the same between selected/unselected states, with the selection border inset. So if we're using 100x64 for the container (same as the inserter item grids), here's what that would look like:

Please notice that with this approach we'll have jumping photo issue, because the photo has to scale up to 64px when unselected and scale down to 64px - border width - white spacing when selected. I thought we wanted to avoid that 🤔

@geriux
Copy link
Member

geriux commented Aug 31, 2020

While testing on Android I found an overflow issue with the selected state border

This issue is now solved 🎉 Thanks @lukewalczak! Just left a minor comment.

@iamthomasbishop
Copy link

Please notice that with this approach we'll have jumping photo issue, because the photo has to scale up to 64px when unselected and scale down to 64px - border width - white spacing when selected. I thought we wanted to avoid that

@lukewalczak I don't think we want the size of the image to change, it should stay the same size. I think what we want is for the border (or combination of borders -- the 1px blue one and 2px white one, 3px total) to be inset, over the top of the image. @geriux is your comment above referring to a fix for that?

@geriux
Copy link
Member

geriux commented Sep 1, 2020

@geriux is your comment above referring to a fix for that?

Nope, it was related to the border getting cut off on Android =)

@iamthomasbishop
Copy link

@lukewalczak tested the latest build for iOS (from here) and it's looking good to me!

geriux
geriux approved these changes Sep 2, 2020
View reviewed changes
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! Thanks for the changes, tested both iOS and Android (Dark mode included) and it's working as expected. Nice work Luke!

@lukewalczak lukewalczak mentioned this pull request Sep 2, 2020
Merged
2 tasks
@lukewalczak lukewalczak merged commit 7ee00ad into master Sep 3, 2020
@lukewalczak lukewalczak deleted the rnmobile/rounded-image branch September 3, 2020 13:34
@github-actions github-actions bot added this to the Gutenberg 9.0 milestone Sep 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@geriux geriux geriux approved these changes

@ellatrix ellatrix Awaiting requested review from ellatrix

@youknowriad youknowriad Awaiting requested review from youknowriad

@dratwas dratwas Awaiting requested review from dratwas

Assignees

@lukewalczak lukewalczak

Labels
[Block] Image Affects the Image Block Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change)
Projects
None yet
Milestone
Gutenberg 9.0
Development

Successfully merging this pull request may close these issues.

[Mobile App] Introduce rounded style option to Image block
3 participants
@lukewalczak @iamthomasbishop @geriux