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

feat(web,a11y): standardize the FullScreenModal UI #8566

Merged
merged 11 commits into from
Apr 8, 2024

Conversation

ben-basten
Copy link
Member

@ben-basten ben-basten commented Apr 6, 2024

Description

Follow up to #8388, #8469.

Standardized FullScreenModal, using the same style as the BaseModal:

  • header: left aligned text, sentence case, close button
  • consistent colors, modal radius, padding
  • optional logo/icon to the left of the header
  • 3 standard sizes
    • wide: 750px
    • narrow: 450px
    • auto: fits the width of the modal content, up to a maximum of 550px

Other improvements:

  • modals now have a max height based on the viewport size, which enables scrolling for screens with less vertical space
  • In mobile screens, stack the confirm modal buttons vertically

Improved accessibility:

  • added aria-modal and aria-labelledby, so the screen reader now announces the header when the modal is opened
  • added scroll padding to the BaseModal, which prevents focus from being hidden behind the sticky bottom footer

Screenshots

Create user

Before

modal-create-user-before

After - light

modal-create-user-after-light

After - dark

modal-create-user-after-dark

Edit user

Before

modal-edit-user-before

After - light

modal-edit-user-after-light

After - dark

modal-edit-user-after-dark

Confirmation modal

Desktop - dark

modal-confirmation-desktop-dark

Desktop - light

modal-confirmation-desktop-light

Mobile - light

modal-confirmation-mobile-light

Set date of birth

Mobile

Note! Modals with CTA buttons that aren't using the ConfirmationDialogue component do not vertically stack the CTA buttons, and can cause horizontal scrolling in some cases.

modal-birth-mobile

Desktop - dark

modal-birth-dark

Desktop - light

modal-birth-light

Change location

Dark

modal-change-location-desktop-dark

Light

modal-change-location-desktop-light

Mobile

modal-change-location-mobile

Keyboard shortcuts

Desktop

modal-shortcuts-desktop

Mobile

modal-shortcuts-mobile

Select avatar color

Desktop

modal-avatar-desktop

Mobile

modal-avatar-mobile

How Has This Been Tested?

Tested using MacOS+VoiceOver+Firefox.

Checklist:

  • npm run lint (linting via ESLint)
  • npm run format (formatting via Prettier)
  • npm run check:svelte (Type checking via SvelteKit)
  • npm test (unit tests)

* consistent header, padding, close button, and radius as BaseModal
* vertically stacking ConfirmDialogue CTA buttons in narrow screens
* adding aria-modal tags for screen reader
* add viewport-specific height limits on modals, to enable scrolling
* prevent focus from being hidden under sticky content in modals
* standardize FullScreenModal widths using a Prop
@ben-basten ben-basten changed the title Feat/standard modal header v2 feat(web,a11y): standardize the FullScreenModal UI Apr 6, 2024
@alextran1502
Copy link
Contributor

Thank you for another amazing PR. I have some feedback

Can we align this?

image

And also not having the button text on the same line (reduce font size) in for this modal's width?

@ben-basten
Copy link
Member Author

Working on fixing the merge conflicts...

…/standard-modal-header-v2

conflicts:

* slideshow-settings.svelte
* album-options.svelte
* create-user-form.svelte
* map-settings-modal.svelte
@ben-basten
Copy link
Member Author

ben-basten commented Apr 6, 2024

Thank you for another amazing PR. I have some feedback

Can we align this?

And also not having the button text on the same line (reduce font size) in for this modal's width?

Fixed the alignment and button wrapping! There are numerous more modals with this issue - I've ran through and fixed those too.

Screenshots

modal-create-revised

modal-edit-revised

@jrasm91 jrasm91 enabled auto-merge (squash) April 8, 2024 21:00
@jrasm91 jrasm91 merged commit 796c933 into immich-app:main Apr 8, 2024
23 checks passed
@ben-basten ben-basten deleted the feat/standard-modal-header-v2 branch April 11, 2024 19:05
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.

4 participants