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

Add native popover component #2474

Merged
merged 3 commits into from
Apr 21, 2021
Merged

Add native popover component #2474

merged 3 commits into from
Apr 21, 2021

Conversation

stitesExpensify
Copy link
Contributor

@stitesExpensify stitesExpensify commented Apr 19, 2021

Details

On tablets we were using popovers like on a desktop due to screen size and that doesn't work very well in that environment. This ensures that we always show popovers as bottom docked regardless of native platform.

Fixed Issues

#1991 (comment)

Tests/QA

  1. Log in and select a conversation
  2. Click the emoji button
  3. If you are on a native app or mobile web the picker should be bottom docked. Otherwise it should be a popup

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

2021-04-20_09-33-01

@stitesExpensify stitesExpensify requested a review from a team as a code owner April 19, 2021 22:04
@stitesExpensify stitesExpensify self-assigned this Apr 19, 2021
@stitesExpensify stitesExpensify removed the request for review from a team April 19, 2021 22:04
@stitesExpensify
Copy link
Contributor Author

@shawnborton any idea why the styling breaks on ipad? Looks like we do a static 12.5% so i'm not sure why there would be extra space on the right

@shawnborton
Copy link
Contributor

Hmm I think we would need to do some kind of text-align: center in each column - the space on the right is actually just white space from the width of each column, where the emoji is on the left edge.

Copy link
Member

@parasharrajat parasharrajat left a comment

Choose a reason for hiding this comment

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

Interesting! Why an empty file src/components/Popover/index.native.js?

@stitesExpensify
Copy link
Contributor Author

Interesting! Why an empty file src/components/Popover/index.native.js?

Because I'm dumb and forgot to push the actual content 😂

@stitesExpensify
Copy link
Contributor Author

Updated to fix style

Copy link
Contributor

@TomatoToaster TomatoToaster left a comment

Choose a reason for hiding this comment

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

LGTM, let me get someone to double check me.

Comment on lines +14 to +15
// eslint-disable-next-line react/jsx-props-no-spreading
{...props}
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm I wonder if we can get rid of this rule, I see it in so many places in the codebase at this point.

Copy link
Contributor

@Jag96 Jag96 left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@Jag96 Jag96 merged commit 1d451d5 into main Apr 21, 2021
@Jag96 Jag96 deleted the stites-addNativePopover branch April 21, 2021 01:49
@OSBotify
Copy link
Contributor

🚀 Deployed to staging 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@marcaaron
Copy link
Contributor

popovers like on a desktop due to screen size and that doesn't work very well in that environment

Are we planning to fix that and revisit the popover for use on tablets? And if so, is there some context we can leave here about why it's not working in that environment and what specifically is not working?

@stitesExpensify
Copy link
Contributor Author

IDK is having a desktop-like popup a normal pattern on tablet apps?

The specific problems we were having were:

  1. The picker is giant for some reason
  2. The picker is based on the click/tap location of the trigger button. If the keyboard is open and you click the emoji button the keyboard closes, but the popover is still way up at the top of the screen because that's where the button was when you pressed it even though now it's at the bottom.

@marcaaron
Copy link
Contributor

is having a desktop-like popup a normal pattern on tablet apps

Thanks! Yeah, I can't ever remember what the status is on this, but I thought we wanted to have all "wide screens" have more or less the same appearance across platforms. With talk about iPad Pros replacing macs lately it seems like it's becoming more "normal".

Not that we need to do anything about it now. Just thought it would be good to leave some breadcrumbs.

The picker is giant for some reason

That seems kind of unexpected.

The picker is based on the click/tap location of the trigger button. If the keyboard is open and you click the emoji button the keyboard closes, but the popover is still way up at the top of the screen because that's where the button was when you pressed it even though now it's at the bottom.

This also seems like a problem with the Popover. I noticed some other issues related to the popover not re-positioning itself so maybe that problem would be resolved after they are addressed.

@OSBotify
Copy link
Contributor

OSBotify commented May 8, 2021

🚀 Deployed to production in version: 1.0.39-5🚀

platform result
🤖 android 🤖 failure ❌
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

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.

7 participants