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

Fix links ui on landscape iOS (v2) #701

Merged
merged 3 commits into from
Mar 5, 2019
Merged

Conversation

etoledom
Copy link
Contributor

@etoledom etoledom commented Mar 5, 2019

Description

This PR updates the Gutenberg ref to test WordPress/gutenberg#14240.

It fixes an issue on KeyboardAvoidingView, that was adding extra bottom padding on specific circumstances:

  • iOS.
  • Landscape.
  • Keyboard already present.
  • BottomSheet presenting with a TextInput on Autofocus.
  • It also had issues without Autofocus.

With some debugging I noticed a problem on the way the extra padding is calculated, so I made a simplified version of the component that will work well on the BottomSheet.

links-ui-landscape

Note: The BottomSheet is not scrollable (yet).

To test:

  • Check that the Links UI presents properly on Landscape mode.
  • If possible, test WPiOS and Android.
  • Android should work exactly as before.

@etoledom etoledom added this to the v1.1 milestone Mar 5, 2019
@etoledom etoledom self-assigned this Mar 5, 2019
@etoledom etoledom requested a review from pinarol March 5, 2019 15:40
@etoledom
Copy link
Contributor Author

etoledom commented Mar 5, 2019

Continuing the discussion from
#647 (comment)

Thank you for your review @pinarol !

The keyboard hide button stops working after the bottom sheet is closed until I manually go and tap to text in the block again.

That's right, I've noticed that too. Let's create a new issue for this one. 👍

Don't know if this is a bug but when I tap to an outside area just after opening the BottomSheet and with no changes made(because I just want to close the BottomSheet) we add the link anyway using an empty url. I think if the user hadn't entered a url text this means we shouldn't add a link and just close the bottom sheet. wdyt?

I agree that we shouldn't try to add a link if the url text is empty. I'd say that this also deserves its own ticket.

Currently we don't have a save and/or cancel button, so we are saving on dismiss, even if it's tapping in the background or swiping down.
Not sure if this is the best way to "save" in general.

@iamthomasbishop what do you think?

@pinarol
Copy link
Contributor

pinarol commented Mar 5, 2019

Adding the gif that describes the issues:

bottom-sheet-keyboard-close

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! 🎉 Feel free to open issues for my findings and merge this one.

(Tested with both WPiOS and WPAndroid and iOS example app)

@iamthomasbishop
Copy link
Contributor

I agree that we shouldn't try to add a link if the url text is empty. I'd say that this also deserves its own ticket.

Agreed.

Currently we don't have a save and/or cancel button, so we are saving on dismiss, even if it's tapping in the background or swiping down. Not sure if this is the best way to "save" in general.

The settings/input of the bottom sheet should be applied immediately where possible, so regardless of the dismiss method (swipe to hide or tapping the scrim in the example given above) we should be saving the contents. It essentially acts the same as a popover on iOS would.

I also think we should consider a couple things:

  • Tapping return should probably dismiss the sheet altogether. Right now, it just hides the keyboard. If we do this, we should probably rename this ^^ button to done.

  • We should make sure the text input for URL use the URL input type/keyboard.

  • (Maybe for another ticket): We are automatically adding http:// to URLs when the user types a straight URL. If I type wordpress.org, I would expect it to stay as wordpress.org.

@etoledom
Copy link
Contributor Author

etoledom commented Mar 5, 2019

Thank you @pinarol and @iamthomasbishop !
Issues opened: #702 and #703

@etoledom etoledom merged commit a298687 into develop Mar 5, 2019
@etoledom etoledom deleted the fix/links-ui-landscape-ios-v2 branch March 5, 2019 21:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants