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: update media query to work with larger phones #8116

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

DiegoCardoso
Copy link
Contributor

Description

Update the media query that sets the overlay to show on the bottom of the page to work with larger phones, such as the iPhone Pro Max.

For such devices, this current media query is not met leading to the menu overlay being opened at the top-left corner of the screen.

Before

image

After

image

Fixes #8092

Type of change

  • Bugfix
  • Feature

Update the media query that sets the overlay to show on the bottom of
the page to work with larger phones, such as the iPhone Pro Max.

For such devices, this current media query is not met leading to the
menu overlay being opened at the top-left corner of the screen.
Copy link
Member

@web-padawan web-padawan left a comment

Choose a reason for hiding this comment

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

I guess this should be then also updated in other places e.g. vaadin-select might have the same problem?

@DiegoCardoso
Copy link
Contributor Author

DiegoCardoso commented Nov 8, 2024

I guess this should be then also updated in other places e.g. vaadin-select might have the same problem?

vaadin-select uses the same style, so it was covered. But I checked for other elements, and vaadin-date-picker should be updated as well as it was looking a bit broken after this change:

Before

image

After the change only on the menu-overlay styles

image

After updating the media query in vaadin-date-picker

image

I also updated media queries in the notification

@tomivirkki
Copy link
Member

Wouldn't incresing the thresholds just postpone the problem until someone tries with a bit larger device?

Screenshot 2024-11-08 at 16 03 48

@DiegoCardoso
Copy link
Contributor Author

Wouldn't incresing the thresholds just postpone the problem until someone tries with a bit larger device?

That's true. I wasn't able to find a value that would work for most of phones with large screens. One option would be to align the media query, in this case, with the threshold value for the media query used in the context menu to define whether a touch device is a phone or not:

_wideMediaQuery: {
type: String,
value: '(min-device-width: 750px)',
},
};
}

The problem now happens with devices that are in between the current 420px and this 750px mentioned on the code above.

@tomivirkki
Copy link
Member

Maybe it would make sense to align the context menu's behavior with the other components and instead of the current _wideMediaQuery, use the same _fullscreenMediaQuery that for example date-picker has. _touch could probably also be removed because the resulting _phone is computed from !wide && touch

@DiegoCardoso DiegoCardoso force-pushed the fix/menu-bar/overlay-position-large-phone branch 2 times, most recently from 144ef47 to c383d2b Compare November 22, 2024 11:10
@DiegoCardoso DiegoCardoso force-pushed the fix/menu-bar/overlay-position-large-phone branch from c383d2b to 875f6a1 Compare November 22, 2024 11:11
Copy link

sonarcloud bot commented Nov 22, 2024

@DiegoCardoso
Copy link
Contributor Author

Maybe it would make sense to align the context menu's behavior with the other components and instead of the current _wideMediaQuery, use the same _fullscreenMediaQuery that for example date-picker has. _touch could probably also be removed because the resulting _phone is computed from !wide && touch

I aligned the phone detection as suggested.

At the same time, I increased the breakpoint to 450px in a few places as this would be enough to accommodate most phones (on my tests, the widest phone - Google Pixel 9 Pro XL - has a width of 448px). It was discussed internally to, at least, do this bump for now and return #83 as part of the next major release (where we can define a better breakpoint system and, perhaps, replace some @media with @container queries).

I decided to rollback changes in notification that I had done previously as it was not directly affected by the changes done as part of this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[menu-bar] sub-menu overlay wrongly placed in some phones
3 participants