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: Link image setting #13654

Merged
merged 40 commits into from
Feb 5, 2019
Merged

Mobile: Link image setting #13654

merged 40 commits into from
Feb 5, 2019

Conversation

etoledom
Copy link
Contributor

@etoledom etoledom commented Feb 4, 2019

Description

This PR implements the Link setting on the image block, using the BottomSheet component.

link-setting

Since on Mobile we have just a textfield (as in Aztec), when the user edit a URL in mobile, it will be automatically changed to custom.

screen shot 2019-02-04 at 12 56 10 pm

This PR also adds some fine tune to the BottomSheet design.

To test:
WPiOS:

  • Build and run the WordPress iOS project (from branch `develop)
  • Checkout this branch from gutenberg-mobile and yarn start.
  • Create a new post using Gutenberg on WPiOS.
  • Add an image to the post.
  • Add a URL (any) to the image using the settings button (wheel button on the inline toolbar).
  • Publish the post.

GB Web

  • Go to WP web and open the post to be edited with Gutenberg.
  • Check that the Custom URL is there.
  • Change the image URL from Custom URL to Media File
  • Save (publish changes).

WPiOS again:

  • Back to WPiOS, close the post and refresh the post list (to get the last changes from web).
  • Open the post again.
  • Check that the URL was updated.
  • Edit the URL and save (publish).
  • Reload the post on the web.
  • Check that the URL changed and its marked as Custom URL.

@etoledom
Copy link
Contributor Author

etoledom commented Feb 5, 2019

Thanks for the review @pinarol !

I pushed a commit with one of the fixes. About the colors, I think that we should organize better how do we work with global color declarations in mobile. Since we use many of those in gutenberg it might be worth it to define all of them there. But I don't think that's work for this PR.

Currently "Reset to Original" button does nothing,

It doesn't :)
It's part of the design, and I added it early to check the cell design, but it's not connected to anything yet.

URL types.

In web, the url type is a picker, and the only one editable by the user is the custom option. The media option is not editable on web, and none gives no option for adding a link.

On web, I can set the URL to media and it will automatically fill the field with the image URL, then I can switch to custom and leave the image URL there, change it a bit or delete it completely, and it will continue being custom.

Since we don't have the picker yet in mobile, and since it's always editable, I think it makes sense to automatically change it to custom, if the user customize it in any way. Still, I'll mention this to Thomas to be sure and we can modify this behavior in the future 👍

@pinarol
Copy link
Contributor

pinarol commented Feb 5, 2019

Alright, I thought we'd be breaking the result url here because custom url's are appended at the end of the site url. But as far as I see it handles redirection pretty well and our result link won't be broken even if we change the link type as custom while leaving the image url as is. So it looks like we don't have to worry for now.

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.

thanks @etoledom LGTM! 🎉

one optional thing: it was hard to select the whole url text and change it, there's no "select all" option available here:

screen shot 2019-02-05 at 20 39 16

Maybe we can tackle and see why "select all" does not appear.

Or maybe we can select the whole text when user focuses on the url. Just an idea.

@etoledom
Copy link
Contributor Author

etoledom commented Feb 5, 2019

Thank you @pinarol !

I did a fast check and there is the Select All option. 🤔

screen shot 2019-02-05 at 7 23 55 pm

It doesn't appear when there is already text selected, but I think that is normal behavior, right?

@etoledom etoledom merged commit a8af1a9 into master Feb 5, 2019
@etoledom etoledom deleted the rnmobile/link-image-setting branch February 5, 2019 18:29
daniloercoli added a commit that referenced this pull request Feb 5, 2019
…rnmobile/372-fix-image-caption-font-family

* 'master' of https://github.com/WordPress/gutenberg:
  Update CODEOWNERS
  Project: Avoid additional native-file code owner notification (#13675)
  Mobile: Link image setting (#13654)

# Conflicts:
#	packages/block-library/src/image/styles.native.scss
@pinarol
Copy link
Contributor

pinarol commented Feb 6, 2019

It doesn't appear when there is already text selected, but I think that is normal behavior, right?

actually when I test this in other text fields I see "select all" even if a small amount of text is selected. maybe that's the case where there are multiple words. I feel like we are having different experience maybe because I had a link that is longer than the text field width and didn't have the option to long press to somewhere outside of the text. but knowing that this option is visible to you it looks like it is an ios behavior and we don't have something to fix here 🤷‍♀️

@youknowriad youknowriad added this to the 5.1 (Gutenberg) milestone Feb 15, 2019
youknowriad pushed a commit that referenced this pull request Mar 6, 2019
* rnmobile: Implement image settings button using InspectorControls.Slot pattern.

* rnmobile: Add missing semicolon

* rnmobile: Adding bottom-sheet component to mobile

* rnmobile: Styling bottom-sheet header

* rnmobile: Bottom-sheet clean up

* rnmobile: Fix lint issues on bottom-sheet related code.

* rnmobile: Fix small lint issues

* rnmobile: Move inline toolbar button definition out of constant.

* rnmobile: Remove extra white-spaces

* revert package-lock.json changes

* rnmobile: Fix merge issue

* rnmobile: Imported BaseControl and TextinputControl to be used on Alt Image Settings

* rnmobile: exporting component BottomSheet.Button to be used as bottom-sheet header buttons

* rnmobile: Adding BottomSheet.Cell component as an extraction for BottomSheet users.

* Fix lint issues

* Reverting changes to package-lock.json

* Fix merge issues

* Remove Done button from Image settings bottom sheet

* Make bottom-sheet avoid being behind keyboard

* Fix lint issues

* Making BottomSheet.Cell value editable as textinput.

* Remove unnecesary onPress prop from Alt cell.

* Image Settings: Added Link To setting to bottom-sheet

* BottomSheet desing details

* Fix bottom-sheet cell value growing larger than container

* Bottom-sheet: Fix bottom safe-area inset with keyboard showing

* Fix lint issues

* Adding textinput props to bottom-sheet cell value.

* Removing autocorrect from link-setting text input.

* Fix lint issues

* Fixing label texts on Image Settings bottom-sheet

* Mobile ImageEdit color using global definition.
youknowriad pushed a commit that referenced this pull request Mar 6, 2019
* rnmobile: Implement image settings button using InspectorControls.Slot pattern.

* rnmobile: Add missing semicolon

* rnmobile: Adding bottom-sheet component to mobile

* rnmobile: Styling bottom-sheet header

* rnmobile: Bottom-sheet clean up

* rnmobile: Fix lint issues on bottom-sheet related code.

* rnmobile: Fix small lint issues

* rnmobile: Move inline toolbar button definition out of constant.

* rnmobile: Remove extra white-spaces

* revert package-lock.json changes

* rnmobile: Fix merge issue

* rnmobile: Imported BaseControl and TextinputControl to be used on Alt Image Settings

* rnmobile: exporting component BottomSheet.Button to be used as bottom-sheet header buttons

* rnmobile: Adding BottomSheet.Cell component as an extraction for BottomSheet users.

* Fix lint issues

* Reverting changes to package-lock.json

* Fix merge issues

* Remove Done button from Image settings bottom sheet

* Make bottom-sheet avoid being behind keyboard

* Fix lint issues

* Making BottomSheet.Cell value editable as textinput.

* Remove unnecesary onPress prop from Alt cell.

* Image Settings: Added Link To setting to bottom-sheet

* BottomSheet desing details

* Fix bottom-sheet cell value growing larger than container

* Bottom-sheet: Fix bottom safe-area inset with keyboard showing

* Fix lint issues

* Adding textinput props to bottom-sheet cell value.

* Removing autocorrect from link-setting text input.

* Fix lint issues

* Fixing label texts on Image Settings bottom-sheet

* Mobile ImageEdit color using global definition.
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)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants