-
-
Notifications
You must be signed in to change notification settings - Fork 828
Pin the action buttons to the bottom of the scrollable dialogs #11407
Pin the action buttons to the bottom of the scrollable dialogs #11407
Conversation
Signed-off-by: Dominik Henneke <dominik.henneke@nordeck.net>
Won't this cause the dialog's height to change if the contents changes? iirc @nadonomy had opinions in this area |
The (ongoing) concern to avoid is resizing modals:
We solved this during login by instead anchoring the modal to the top of the screen so it can grow/shrink between itself. This means it's not vertically centred but is often offset up, but that's also desirable. I can't speak to how wide reaching this PR is (is it all modals?), so would defer to yourself @t3chguy or @germain-gg (thinking about componentising in Compound) on the best next steps here. |
@nadonomy it affects Poll creation dialogs and Module API dialogs, as well as any future dialogs which extend the (newer) ScrollableBaseModal class |
Got it. In that case @germain-gg wdyt? Should we optimise for the resizing/anchoring concerns in this PR now, or further down the line as we mature and adopt components in Compound?
|
Yes that can happen, but it's a very extreme min-height example. WCAG 2.1 advises to at least have it usable at 256px height which is okay-ish, (though unrelated for this change): About the potential layout shift in the layout: The authors of the component would need to be cautious about the content they put in it. Not sure if re-layouting would actually be a problem if it at least does happen based on a user action. In case you would prefer to not change the height, we should at least make sure the buttons are always aligned to the bottom of the dialog. Let me know what you would prefer. |
This sound good. I think we should set a minimum height on the modal - the 256px/WCAG guidance seems sensible to me. |
Signed-off-by: Dominik Henneke <dominik.henneke@nordeck.net>
I updated the PR to pin the buttons to the bottom of the dialog instead of letting it shrink. I would prefer to not work on the minimal-height-issues in this PR. It's a separate topic and one will inevitably run into situations where one will get multiple scrollbars. As written above, with the 256px total page height, at least no content is overflowing, though it is not perfect. But I think that is something that the design system would need to discuss and handle. |
@dhenneke my sense is the same, just waiting on @germain-gg to confirm if this PR has any unintended consequences (e.g. if this modal component is consumed for multi-step modals, and these changes impact placement between screens). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks good to me.
To answer @nadonomy it seems to be used only for
- Poll creation dialogs
- Link edits in the rich text editor
Changes in [1.11.40](https://github.com/vector-im/element-web/releases/tag/v1.11.40) (2023-08-29) ================================================================================================= ## ✨ Features * Hide account deactivation for externally managed accounts ([\#11445](matrix-org/matrix-react-sdk#11445)). Fixes #26022. Contributed by @kerryarchibald. * OIDC: Redirect to delegated auth provider when signing out ([\#11432](matrix-org/matrix-react-sdk#11432)). Fixes #26000. Contributed by @kerryarchibald. * Disable 3pid fields in settings when `m.3pid_changes` capability is disabled ([\#11430](matrix-org/matrix-react-sdk#11430)). Fixes #25995. Contributed by @kerryarchibald. * OIDC: disable multi session signout for OIDC-aware servers in session manager ([\#11431](matrix-org/matrix-react-sdk#11431)). Contributed by @kerryarchibald. * Implement updated open dialog method of the Module API ([\#11395](matrix-org/matrix-react-sdk#11395)). Contributed by @dhenneke. * Polish & delabs `Exploring public spaces` feature ([\#11423](matrix-org/matrix-react-sdk#11423)). * Treat lists with a single empty item as plain text, not Markdown. ([\#6833](matrix-org/matrix-react-sdk#6833)). Fixes element-hq/element-meta#1265. * Allow managing room knocks ([\#11404](matrix-org/matrix-react-sdk#11404)). Contributed by @charlynguyen. * Pin the action buttons to the bottom of the scrollable dialogs ([\#11407](matrix-org/matrix-react-sdk#11407)). Contributed by @dhenneke. * Support Matrix 1.1 (drop legacy r0 versions) ([\#9819](matrix-org/matrix-react-sdk#9819)). ## 🐛 Bug Fixes * Fix path separator for Windows based systems ([\#25997](element-hq/element-web#25997)). * Fix instances of double translation and guard translation calls using typescript ([\#11443](matrix-org/matrix-react-sdk#11443)). * Fix export type "Current timeline" to match its behaviour to its name ([\#11426](matrix-org/matrix-react-sdk#11426)). Fixes #25988. * Fix Room Settings > Notifications file upload input being shown superfluously ([\#11415](matrix-org/matrix-react-sdk#11415)). Fixes #18392. * Simplify registration with email validation ([\#11398](matrix-org/matrix-react-sdk#11398)). Fixes #25832 #23601 and #22297. * correct home server URL ([\#11391](matrix-org/matrix-react-sdk#11391)). Fixes #25931. Contributed by @NSV1991. * Include non-matching DMs in Spotlight recent conversations when the DM's userId is part of the search API results ([\#11374](matrix-org/matrix-react-sdk#11374)). Contributed by @mgcm. * Fix useRoomMembers missing updates causing incorrect membership counts ([\#11392](matrix-org/matrix-react-sdk#11392)). Fixes #17096. * Show error when searching public rooms fails ([\#11378](matrix-org/matrix-react-sdk#11378)).
This fixes the location of the action buttons in the dialog that is used by the Module API, for example by the ILAG module.
It would also be nice if the "shadow" would disappear when the content is not actually scrolling, but I didn't have a good idea on how to solve that easily. For now, the size is our main concern.
Checklist
Type: enhancement
Here's what your changelog entry will look like:
✨ Features