Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

fix(underlay): dispatch a "close" event rather than relying naively on "click" #4020

Merged
merged 1 commit into from
Feb 8, 2024

Conversation

Westbrook
Copy link
Contributor

Description

Relying on click directly means that patterns that open an Overlay on pointerdown, like <sp-picker>, could inadvertently close the Overlay as part of that initial interaction.

How has this been tested?

  • Test case 1
    1. Go here
    2. Use a mobile device, emulator, or Dev Tools to mock the mobile context
    3. Open one of the theme Pickers
    4. See that it stays open.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)

Checklist

  • I have signed the Adobe Open Source CLA.
  • My code follows the code style of this project.
  • If my change required a change to the documentation, I have updated the documentation in this pull request.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@Westbrook Westbrook requested a review from a team February 8, 2024 14:36
Copy link

github-actions bot commented Feb 8, 2024

Tachometer results

Chrome

action-menu permalink

Version Bytes Avg Time vs remote vs branch
npm latest 656 kB 167.77ms - 171.19ms - unsure 🔍
-1% - +2%
-1.51ms - +3.69ms
branch 646 kB 166.43ms - 170.35ms unsure 🔍
-2% - +1%
-3.69ms - +1.51ms
-

dialog permalink

Version Bytes Avg Time vs remote vs branch
npm latest 515 kB 82.41ms - 83.91ms - faster ✔
0% - 4%
0.25ms - 3.24ms
branch 505 kB 83.61ms - 86.20ms slower ❌
0% - 4%
0.25ms - 3.24ms
-

picker permalink

Version Bytes Avg Time vs remote vs branch
npm latest 517 kB 599.18ms - 607.37ms - unsure 🔍
-1% - +1%
-5.29ms - +6.47ms
branch 509 kB 598.47ms - 606.91ms unsure 🔍
-1% - +1%
-6.47ms - +5.29ms
-

split-button permalink

Version Bytes Avg Time vs remote vs branch
npm latest 723 kB 1859.22ms - 1862.50ms - unsure 🔍
-0% - +0%
-0.51ms - +3.76ms
branch 713 kB 1857.86ms - 1860.60ms unsure 🔍
-0% - +0%
-3.76ms - +0.51ms
-

tray permalink

Version Bytes Avg Time vs remote vs branch
npm latest 529 kB 103.55ms - 104.47ms - unsure 🔍
-0% - +1%
-0.52ms - +0.53ms
branch 520 kB 103.75ms - 104.25ms unsure 🔍
-1% - +0%
-0.53ms - +0.52ms
-

underlay permalink

Version Bytes Avg Time vs remote vs branch
npm latest 363 kB 13.17ms - 13.28ms - faster ✔
1% - 2%
0.13ms - 0.31ms
branch 355 kB 13.37ms - 13.51ms slower ❌
1% - 2%
0.13ms - 0.31ms
-
Firefox

action-menu permalink

Version Bytes Avg Time vs remote vs branch
npm latest 656 kB 330.14ms - 343.22ms - unsure 🔍
-2% - +3%
-5.12ms - +10.56ms
branch 646 kB 329.64ms - 338.28ms unsure 🔍
-3% - +2%
-10.56ms - +5.12ms
-

dialog permalink

Version Bytes Avg Time vs remote vs branch
npm latest 515 kB 138.61ms - 145.63ms - unsure 🔍
-2% - +5%
-2.92ms - +6.40ms
branch 505 kB 137.32ms - 143.44ms unsure 🔍
-4% - +2%
-6.40ms - +2.92ms
-

picker permalink

Version Bytes Avg Time vs remote vs branch
npm latest 517 kB 988.52ms - 1019.80ms - unsure 🔍
-1% - +2%
-8.68ms - +23.92ms
branch 509 kB 991.95ms - 1001.13ms unsure 🔍
-2% - +1%
-23.92ms - +8.68ms
-

split-button permalink

Version Bytes Avg Time vs remote vs branch
npm latest 723 kB 1571.16ms - 1574.80ms - unsure 🔍
+0% - +0%
+0.33ms - +5.71ms
branch 713 kB 1567.98ms - 1571.94ms unsure 🔍
-0% - -0%
-5.71ms - -0.33ms
-

tray permalink

Version Bytes Avg Time vs remote vs branch
npm latest 529 kB 194.11ms - 201.01ms - unsure 🔍
-3% - +2%
-6.36ms - +4.12ms
branch 520 kB 194.73ms - 202.63ms unsure 🔍
-2% - +3%
-4.12ms - +6.36ms
-

underlay permalink

Version Bytes Avg Time vs remote vs branch
npm latest 363 kB 39.96ms - 44.24ms - unsure 🔍
-11% - +3%
-5.05ms - +1.29ms
branch 355 kB 41.64ms - 46.32ms unsure 🔍
-3% - +12%
-1.29ms - +5.05ms
-

@hunterloftis
Copy link
Contributor

I'm just using emulated mobile behaviors in Chrome, which isn't always 100% accurate, but the picker doesn't stay open in this case:

mobile-picker.mov

@Westbrook
Copy link
Contributor Author

@hunterloftis that is NOT the experience that I was addressing, but I'll dig into that, too! 😳

In a mobile context, so smaller window but still touch, a Tray will be opened with an Underlay, instead of the Popover. That is hopefully addressed by this change. Will report back when I know more about touch more broadly.

hunterloftis
hunterloftis previously approved these changes Feb 8, 2024
Copy link
Contributor

@hunterloftis hunterloftis left a comment

Choose a reason for hiding this comment

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

Ah I see, yep repro'd both the existing & the fixed behavior.

packages/underlay/src/Underlay.ts Outdated Show resolved Hide resolved
najikahalsema
najikahalsema previously approved these changes Feb 8, 2024
Copy link
Collaborator

@najikahalsema najikahalsema left a comment

Choose a reason for hiding this comment

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

Reproduced successfully on my iPhone 👍 Looks good.

@Westbrook Westbrook dismissed stale reviews from najikahalsema and hunterloftis via 241e661 February 8, 2024 22:12
@Westbrook Westbrook merged commit 4f9aa4a into main Feb 8, 2024
47 checks passed
@Westbrook Westbrook deleted the picker-mobile branch February 8, 2024 22:23
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants