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

[RNmobile] Improve "take video" icon #20927

Merged
merged 12 commits into from
Mar 18, 2020
Merged

Conversation

hypest
Copy link
Contributor

@hypest hypest commented Mar 16, 2020

Description

This PR improves the "Take a Video" icon we're using on the media selection bottom sheet. So far, we've been using the "camera" icon, the very same as the "Take a Photo" icon, which is apparently less than ideal to be the same.

Screenshot 2020-03-16 at 15 26 23

How has this been tested?

Use this gutenberg-mobile PR: wordpress-mobile/gutenberg-mobile#2024

To test:

  1. Run the native mobile demo app and add a Media&Text block
  2. Tap on the "Add image or video" button in the media area of the block
  3. Notice the video-alt2 icon being used for the "Take a video" option

Types of changes

  1. Introducing the "video-alt2" icon, ported from the corresponding Dashicon https://github.com/WordPress/dashicons/blob/master/sources/svg/video-alt2.svg. Adding you @youknowriad as a reviewer on this PR for the web-related changes.
  2. I'm not sure if I followed the proper procedure to port the icon. I copy/pasted the SVG path but I also moved the icon a bit to the right of its otherwise horizontal center, as it wasn't visually balanced in the very center. Used Inkscape for that movement. @jasmussen maybe you have some clue here whether this looks OK or not, or perhaps there's a better way of porting the icon from Dashicons.
  3. Using the new icon on the native mobile bottom sheet when selecting media

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.

@hypest hypest added Needs Design Feedback Needs general design feedback. Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change) labels Mar 16, 2020
@hypest hypest requested review from youknowriad and ceyhun March 16, 2020 13:53
@github-actions
Copy link

github-actions bot commented Mar 16, 2020

Size Change: 0 B

Total Size: 856 kB

ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 998 B 0 B
build/annotations/index.js 3.43 kB 0 B
build/api-fetch/index.js 3.39 kB 0 B
build/autop/index.js 2.58 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/index.js 6.02 kB 0 B
build/block-directory/style-rtl.css 760 B 0 B
build/block-directory/style.css 760 B 0 B
build/block-editor/index.js 100 kB 0 B
build/block-editor/style-rtl.css 10.9 kB 0 B
build/block-editor/style.css 10.9 kB 0 B
build/block-library/editor-rtl.css 7.23 kB 0 B
build/block-library/editor.css 7.24 kB 0 B
build/block-library/index.js 111 kB 0 B
build/block-library/style-rtl.css 7.42 kB 0 B
build/block-library/style.css 7.43 kB 0 B
build/block-library/theme-rtl.css 669 B 0 B
build/block-library/theme.css 671 B 0 B
build/block-serialization-default-parser/index.js 1.65 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/blocks/index.js 57.5 kB 0 B
build/components/index.js 191 kB 0 B
build/components/style-rtl.css 15.7 kB 0 B
build/components/style.css 15.7 kB 0 B
build/compose/index.js 6.21 kB 0 B
build/core-data/index.js 10.6 kB 0 B
build/data-controls/index.js 1.04 kB 0 B
build/data/index.js 8.2 kB 0 B
build/date/index.js 5.37 kB 0 B
build/deprecated/index.js 772 B 0 B
build/dom-ready/index.js 568 B 0 B
build/dom/index.js 3.06 kB 0 B
build/edit-post/index.js 91.2 kB 0 B
build/edit-post/style-rtl.css 8.47 kB 0 B
build/edit-post/style.css 8.46 kB 0 B
build/edit-site/index.js 5.07 kB 0 B
build/edit-site/style-rtl.css 2.53 kB 0 B
build/edit-site/style.css 2.53 kB 0 B
build/edit-widgets/index.js 4.43 kB 0 B
build/edit-widgets/style-rtl.css 2.58 kB 0 B
build/edit-widgets/style.css 2.58 kB 0 B
build/editor/editor-styles-rtl.css 381 B 0 B
build/editor/editor-styles.css 382 B 0 B
build/editor/index.js 43.8 kB 0 B
build/editor/style-rtl.css 3.97 kB 0 B
build/editor/style.css 3.96 kB 0 B
build/element/index.js 4.45 kB 0 B
build/escape-html/index.js 733 B 0 B
build/format-library/index.js 6.96 kB 0 B
build/format-library/style-rtl.css 502 B 0 B
build/format-library/style.css 502 B 0 B
build/hooks/index.js 1.93 kB 0 B
build/html-entities/index.js 621 B 0 B
build/i18n/index.js 3.49 kB 0 B
build/is-shallow-equal/index.js 710 B 0 B
build/keyboard-shortcuts/index.js 2.3 kB 0 B
build/keycodes/index.js 1.69 kB 0 B
build/list-reusable-blocks/index.js 2.99 kB 0 B
build/list-reusable-blocks/style-rtl.css 226 B 0 B
build/list-reusable-blocks/style.css 226 B 0 B
build/media-utils/index.js 4.84 kB 0 B
build/notices/index.js 1.58 kB 0 B
build/nux/index.js 3.01 kB 0 B
build/nux/style-rtl.css 616 B 0 B
build/nux/style.css 613 B 0 B
build/plugins/index.js 2.54 kB 0 B
build/primitives/index.js 1.5 kB 0 B
build/priority-queue/index.js 780 B 0 B
build/redux-routine/index.js 2.84 kB 0 B
build/rich-text/index.js 14.3 kB 0 B
build/server-side-render/index.js 2.55 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.01 kB 0 B
build/viewport/index.js 1.61 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.18 kB 0 B

compressed-size-action

Copy link
Member

@ceyhun ceyhun left a comment

Choose a reason for hiding this comment

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

Just tested on an Android 9 device and it looks good 👍 A snapshot test seems to be failing on Travis though

@jasmussen jasmussen self-requested a review March 16, 2020 15:03
Copy link
Contributor

@jasmussen jasmussen left a comment

Choose a reason for hiding this comment

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

I'll provide a new icon and a new name as soon as I can!

@jasmussen
Copy link
Contributor

Can you try this icon?

<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 24 24"><path d="M16 9V7c0-.6-.4-1-1-1H4c-.6 0-1 .4-1 1v10c0 .6.4 1 1 1h11c.6 0 1-.4 1-1v-2l5 2.5v-11L16 9zm3.5 6.1l-5-2.5v3.9h-10v-9h10v3.9l5-2.5v6.2z"/></svg>

Screenshot 2020-03-16 at 16 42 49

Call it capture-video or captureVideo.

@pablohoneyhoney
Copy link

Here another version

icon-capture-video

Figma

@jasmussen
Copy link
Contributor

Here's the SVG for Pablo's improved version:

<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 24 24"><path d="M14 6H5c-1.1 0-2 .9-2 2v8c0 1.1.9 2 2 2h9c1.1 0 2-.9 2-2V8c0-1.1-.9-2-2-2zm.5 10c0 .3-.2.5-.5.5H5c-.3 0-.5-.2-.5-.5V8c0-.3.2-.5.5-.5h9c.3 0 .5.2.5.5v8zm2.5-6v4l5 3V7l-5 3zm3.5 4.4l-2-1.2v-2.3l2-1.2v4.7z"/></svg>

@hypest
Copy link
Contributor Author

hypest commented Mar 17, 2020

Thanks for the new icon @jasmussen and @pablohoneyhoney !

Looks like the new icon has a different style than the rest on the sheet though (the new one is "hollow". See below.
screenshot-1584433634254

Can we have a "solid" version of the icon for this dialog? We could try to replace the other icons too but that starts to feel outside the scope of this PR.

@jasmussen
Copy link
Contributor

Looks like the new icon has a different style than the rest on the sheet though (the new one is "hollow". See below.

Honestly, I think the icons for "Choose from devic" and "Take a photo" are so classic looking that they are the ones standing out.

If you like, I can provide two icons for those, and you can update them as well? Or you can do that in separate PRs?

@jasmussen
Copy link
Contributor

The "Choose from device" can use the image icon, currently used for the Image block.

Here's a new icon for taking photos. Can call it capture-photo or capturePhoto:

<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 24 24"><path d="M12 8.1c-2.2 0-3.9 1.8-3.9 3.9s1.8 4 3.9 4 4-1.8 4-4-1.8-3.9-4-3.9zm0 6.4c-1.4 0-2.4-1.1-2.4-2.5s1.1-2.4 2.4-2.4 2.5 1.1 2.5 2.4-1.1 2.5-2.5 2.5zM20 5h-3.8l-1.7-2h-5L7.8 5H4c-1.1 0-2 .9-2 2v10c0 1.1.9 2 2 2h16c1.1 0 2-.9 2-2V7c0-1.1-.9-2-2-2zm.5 12c0 .3-.2.5-.5.5H4c-.3 0-.5-.2-.5-.5V7c0-.3.2-.5.5-.5h4.5l1.7-2h3.6l1.7 2H20c.3 0 .5.2.5.5v10z"/></svg>

Screenshot 2020-03-17 at 14 49 24

@hypest
Copy link
Contributor Author

hypest commented Mar 17, 2020

Thanks @jasmussen ! Fair, let's replace the other icons too, in this PR. I'll use the capture-photo one and will also use the "choose from device" when you have it ready, thanks for the work here!

Ah, and cc @iamthomasbishop as a heads up that we'll change the icons on that bottom sheet.

@iamthomasbishop
Copy link

Looks great, @jasmussen! Thank you!

@hypest
Copy link
Contributor Author

hypest commented Mar 17, 2020

Updated the capture-photo icon, here's how it looks:

screenshot-1584458740055

@jasmussen
Copy link
Contributor

jasmussen commented Mar 17, 2020

Need the image one as well :) It's in the Icon package, called image.

The one on the left here:

Screenshot 2020-03-17 at 16 33 08

@hypest
Copy link
Contributor Author

hypest commented Mar 17, 2020

All updated now, here's how it looks:

screenshot-1584459468767

@jasmussen jasmussen self-requested a review March 17, 2020 15:45
Copy link
Contributor

@jasmussen jasmussen left a comment

Choose a reason for hiding this comment

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

Nice.

@pablohoneyhoney
Copy link

Here a new iteration to pair width and shapes a bit.

Screen Shot 2020-03-17 at 5 37 57 PM

@jasmussen
Copy link
Contributor

Thanks for the vectors, Pablo!

I updated the vectors (and the name, it was captureCamera instead of capturePhoto :) ), lets's see if the checks pass.

@hypest
Copy link
Contributor Author

hypest commented Mar 18, 2020

Thanks for the updates Pablo and Joen!

I went ahead and updated from master (and develop on wordpress-mobile/gutenberg-mobile#2024) and all is looking good. I'll merge when CI gets green.

Here's how the bottom sheet looks like at the moment:

screenshot-1584541055892

@hypest hypest merged commit 8c13651 into master Mar 18, 2020
@hypest hypest deleted the rnmobile/improve-take-video-icon branch March 18, 2020 15:56
@github-actions github-actions bot added this to the Gutenberg 7.8 milestone Mar 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants