search

bcc-code / bcc-design

Packages, assets and documentation to use the BCC Design System
https://developer.bcc.no/bcc-design
Apache License 2.0
3 stars 6 forks source link

fix: modal for mobile #265

Closed Bernton closed 10 months ago

Bernton commented 11 months ago

Change summary

use 100dvw and 100dvh instead of w-screen and h-screen

Without this, the dialog will overflow on mobile browser, because they dynamically display navigation bar when users scroll up/down. "w-screen" and "h-screen" (vw, vh) don't react correctly to this.

Change type

github-actions[bot] commented 11 months ago

Azure Static Web Apps: Your stage site is ready! Visit it here: https://brave-coast-028fe6203-265.westeurope.3.azurestaticapps.net

Bernton commented 11 months ago

Not sure why the snapshot is different actually, on non-mobile browsers, there shouldn't be a difference.

laurensgroeneveld commented 11 months ago

The snapshot just looks at the outputted HTML, and this change updates some classes in the HTML so the snapshot is not the same anymore.

Bernton commented 11 months ago

Shouldn't the snapshot only account for visual changes?

laurensgroeneveld commented 11 months ago

These are just the snapshots from Vitest, in the unit tests. You can see the output of it as they're checked into Git: https://github.com/bcc-code/bcc-design/blob/main/design-library/src/components/BccModal/__snapshots__/BccModal.spec.ts.snap

Their utility in that regard is limited, but I think they still can be nice as changes to the HTML often indicate that there might be some change that needs to be added to the release notes, as they're important for people that use the CSS library. Additionally I ran into it before when updating the button that it also touches components that internally use the button component, reminding me to check them.

Comparing visual snapshots would also be very nice but is on a very different level. There are tools for it (Chromatic is one, from the makers of Storybook) but it's more involved as it essentially requires spinning up a browser. When I review a PR I do this manually by comparing the production Storybook with the output of the PR.

Bernton commented 11 months ago

@u12206050 raised the point that dvw and dvh might not have wide enough support among browsers, what do you think? @laurensgroeneveld

laurensgroeneveld commented 11 months ago

I thought so as well but according to caniuse the support is great: https://caniuse.com/mdn-css_types_length_viewport_percentage_units_dynamic

u12206050 commented 11 months ago

Support is good, but it doesn't yet cover the last three versions.

I suggested we use env(safe-area-inset-bottom) which does have better support, and works fine for the event app.

Probably this time next year we can use dvw

github-actions[bot] commented 10 months ago

Azure Static Web Apps: Your stage site is ready! Visit it here: https://brave-coast-028fe6203-265.westeurope.3.azurestaticapps.net