adding some composition options for modals #15084

selfcontained · 2022-11-30T22:06:51Z

Description

The intention here is to fix the linked issue below so modals are more responsive, and the body of a modal can shrink and scroll when needed. To accomplish that, I needed to add a few css classes to the modal container and body wrapper. In order to allow us to have these sections of a modal consistently rendered, I created a few sub-components so we can compose our Modals a bit as well. <Modal/>s can still use the title prop and buttons prop, which will get wrapped automatically like they were. It might be a good idea to unify how we render modals though at some point, and always compose them like below, but not an urgent change we have to make. These changes should provide a responsive modal w/ scrollable body for any pre-existing modal that was passing in a title prop as well.

I've also moved the classnames dependency from devDeps to normal deps as I wanted to use it for some conditional class names. I'm happy to adjust that back though if we explicitly don't want to use classnames outside of tests though.

<Modal ...>
  <ModalHeader ...>
  <ModalContent ...>
  <ModalFooter ...>
</Modal>

We can iterate on this, and add built-in behavior to support pinned error messages on a modal that I saw in at least one place (when adding a new integration). This would allow that style of modal to also render responsively for smaller screen sizes.

Related Issue(s)

Fixes #12899, #10747

How to test

Navigate to Settings/Integrations and edit one of the integrations to open the modal.

Resize your browser window to be smaller than the height of the modal, and you should see it shrink, while being able to scroll the inner contents of the modal.

* I've applied the same updates to the Environment Variables modal.

Release Notes

Fixes an issue with modals not displaying properly on smaller screens.

Documentation

Werft options:

/werft with-local-preview
If enabled this will build install/preview
/werft with-preview
/werft with-large-vm
/werft with-integration-tests=all
Valid options are all, workspace, webapp, ide, jetbrains, vscode, ssh

werft-gitpod-dev-com · 2022-11-30T22:07:00Z

started the job as gitpod-build-bmh-scrollable-modal-content.2 because the annotations in the pull request description changed
(with .werft/ from main)

components/dashboard/src/components/Modal.tsx

selfcontained · 2022-12-02T00:03:20Z

components/dashboard/src/components/Modal.tsx

@@ -14,8 +15,8 @@ export default function Modal(props: {
    specify?: string;
    title?: string;
    hideDivider?: boolean;
-    buttons?: React.ReactChild[] | React.ReactChild;
-    children: React.ReactChild[] | React.ReactChild;
+    buttons?: ReactNode;


ReactNode type encompasses arrays or single children, but also allows for null.

selfcontained · 2022-12-02T00:04:11Z

components/dashboard/src/components/Modal.tsx

-                        (props.className || "")
-                    }
+                    className={cn(
+                        "flex flex-col max-h-screen relative bg-white dark:bg-gray-900 border border-gray-200 dark:border-gray-800 rounded-xl p-6 max-w-lg mx-auto text-left ",


this allows the modal container to adjust height, while being height constrained to the screen viewport

easyCZ · 2022-12-02T15:37:02Z

When testing with Brave, on a 4k screen, the modal renders what looks like scroll bars, without actually being scrollable.

selfcontained · 2022-12-02T17:31:23Z

Good catch - I think this is from not using an overflow: auto style (browsers should hide scrollbars unless needed). I'ved updated that scrollable body to use a tailwind class that should remove the scrollbars unless needed (also it should only apply a vertical scrollbar if needed and no horizontal).

easyCZ · 2022-12-05T09:08:14Z

Looking good. Fixes the problem of not being able to scroll the modal.

easyCZ

LGTM

gtsiolis · 2022-12-05T13:09:18Z

Looking at this now! 👀

gtsiolis

Hey @selfcontained! Welcome to the team, and thanks for opening this! ✨

Left a few non-blocking comments below that we can split into separate follow-up issues.

Some thoughts on the UX changes of this PR:

The responsive design of the dashboard is being tracked in Epic: Improve dashboard responsive layout #4050. 💡
Improvements like creating more flexible and compact components like Replace the editor selection cards with the new dropdown component used in the start with options new workspace modal #13116 could help in some cases, see early designs in IDE selection modal window not fiting in screen #10747 (comment). 💡
Generally, we could aim for making the viewport scrollable when the modal content extends below the viewport. 💭
But making the modal contents scroll here sounds like a good, boring (simple) MVC solution here and will also resolve issues like IDE selection modal window not fiting in screen #10747. 🛹
If scrolling is needed, this may be a good indicator to consider opening the content in a new page instead. So far, the modal component and pattern has suited well our needs, and we just started experimenting with opening content in new pages in the new access tokens product are, see /new and /edit flows in /tokens. 💡

Since code changes have been approved from two WebApp team members and there are no blocking UX comments, let's take a bias for action, merge this, and open any follow-up issues if needed. ✔️

gtsiolis · 2022-12-05T13:13:18Z

components/dashboard/src/settings/Integrations.tsx

@@ -291,8 +291,8 @@ function GitProviders() {
            {editModal && (
                // TODO: Use title and buttons props
                <Modal visible={true} onClose={() => setEditModal(undefined)}>
-                    <h3 className="pb-2">Edit Permissions</h3>
-                    <div className="border-t border-b border-gray-200 dark:border-gray-800 mt-2 -mx-6 px-6 py-4">
+                    <ModalHeader>Edit Permissions</ModalHeader>


suggestion: Since you've replaced the modal contents here could we also replace the contents for the New Git integration modal which is also much needed, see L750 below. If this is not as straightforward as the other instances, let's open a follow-up to keep track of this. 🏓

gitpod/components/dashboard/src/settings/Integrations.tsx

Line 750 in 797c1b7

<h3 className="pb-2">{mode === "new" ? "New Git Integration" : "Git Integration"}</h3>

gtsiolis · 2022-12-05T13:18:06Z

components/dashboard/src/components/Modal.tsx

-                        (props.className || "")
-                    }
+                    className={cn(
+                        "flex flex-col max-h-screen relative bg-white dark:bg-gray-900 border border-gray-200 dark:border-gray-800 rounded-xl p-6 max-w-lg mx-auto text-left ",


thought: This will also resolve #10747. I've updated the PR description to also include that issue.

gtsiolis · 2022-12-05T13:26:02Z

components/dashboard/src/components/Modal.tsx

+                            <ModalHeader>{props.title}</ModalHeader>
+                            <ModalBody hideDivider={props.hideDivider}>{props.children}</ModalBody>
+                            <ModalFooter>{props.buttons}</ModalFooter>


praise: Not sure if this introduces unnecessary complexity, but more structure like this is definitely needed! ✨

gtsiolis · 2022-12-05T13:30:32Z

components/dashboard/src/components/Modal.tsx

-                            </div>
-                            <div className="flex justify-end mt-6 space-x-2">{props.buttons}</div>
+                            <ModalHeader>{props.title}</ModalHeader>
+                            <ModalBody hideDivider={props.hideDivider}>{props.children}</ModalBody>


fyi: Not sure where to add this comment, but one modal I'd love to improve after thes changes is the feedback modal. Cross-posting follow up issue (#10307) for visibility.

gtsiolis · 2022-12-05T13:42:14Z

/unhold

adding some composition options for modals

8dd290c

roboquat added do-not-merge/work-in-progress do-not-merge/release-note-label-needed labels Nov 30, 2022

roboquat added the size/M label Nov 30, 2022

AlexTugarev reviewed Dec 1, 2022

View reviewed changes

components/dashboard/src/components/Modal.tsx Show resolved Hide resolved

updating classnames dep and using ReactNode type

9d01bcd

roboquat added size/L and removed size/M labels Dec 1, 2022

selfcontained added the do-not-merge/hold label Dec 1, 2022

selfcontained commented Dec 2, 2022

View reviewed changes

cleanup and apply sub-components when title exists

44d3d87

roboquat added size/M and removed size/L labels Dec 2, 2022

selfcontained marked this pull request as ready for review December 2, 2022 00:11

selfcontained requested a review from a team December 2, 2022 00:11

roboquat removed the do-not-merge/work-in-progress label Dec 2, 2022

github-actions bot added the team: webapp Issue belongs to the WebApp team label Dec 2, 2022

roboquat added release-note and removed do-not-merge/release-note-label-needed labels Dec 2, 2022

adjust to overflow-y-auto

692cc8d

gtsiolis self-requested a review December 2, 2022 17:09

easyCZ approved these changes Dec 5, 2022

View reviewed changes

gtsiolis approved these changes Dec 5, 2022

View reviewed changes

roboquat removed the do-not-merge/hold label Dec 5, 2022

roboquat merged commit 80a1a4f into main Dec 5, 2022

roboquat deleted the bmh/scrollable-modal-content branch December 5, 2022 13:43

gtsiolis mentioned this pull request Dec 5, 2022

IDE selection modal window not fiting in screen #10747

Closed

roboquat added deployed: webapp Meta team change is running in production deployed Change is completely running in production labels Dec 6, 2022

gtsiolis mentioned this pull request Mar 16, 2023

Org SSO Page updates #16868

Merged

14 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

adding some composition options for modals #15084

adding some composition options for modals #15084

selfcontained commented Nov 30, 2022 •

edited by gtsiolis

Loading

werft-gitpod-dev-com bot commented Nov 30, 2022

selfcontained Dec 2, 2022

selfcontained Dec 2, 2022

easyCZ commented Dec 2, 2022

selfcontained commented Dec 2, 2022

easyCZ commented Dec 5, 2022

easyCZ left a comment

gtsiolis commented Dec 5, 2022 •

edited by werft-gitpod-dev-com bot

Loading

gtsiolis left a comment •

edited

Loading

gtsiolis Dec 5, 2022

gtsiolis Dec 5, 2022

gtsiolis Dec 5, 2022

gtsiolis Dec 5, 2022

gtsiolis commented Dec 5, 2022

adding some composition options for modals #15084

adding some composition options for modals #15084

Conversation

selfcontained commented Nov 30, 2022 • edited by gtsiolis Loading

Description

Related Issue(s)

How to test

Release Notes

Documentation

Werft options:

werft-gitpod-dev-com bot commented Nov 30, 2022

selfcontained Dec 2, 2022

Choose a reason for hiding this comment

selfcontained Dec 2, 2022

Choose a reason for hiding this comment

easyCZ commented Dec 2, 2022

selfcontained commented Dec 2, 2022

easyCZ commented Dec 5, 2022

easyCZ left a comment

Choose a reason for hiding this comment

gtsiolis commented Dec 5, 2022 • edited by werft-gitpod-dev-com bot Loading

gtsiolis left a comment • edited Loading

Choose a reason for hiding this comment

gtsiolis Dec 5, 2022

Choose a reason for hiding this comment

gtsiolis Dec 5, 2022

Choose a reason for hiding this comment

gtsiolis Dec 5, 2022

Choose a reason for hiding this comment

gtsiolis Dec 5, 2022

Choose a reason for hiding this comment

gtsiolis commented Dec 5, 2022

selfcontained commented Nov 30, 2022 •

edited by gtsiolis

Loading

gtsiolis commented Dec 5, 2022 •

edited by werft-gitpod-dev-com bot

Loading

gtsiolis left a comment •

edited

Loading