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.

Sign up for GitHub

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

adding some composition options for modals #15084

Merged
merged 4 commits into from
Dec 5, 2022
Merged

adding some composition options for modals #15084

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8dd290c
adding some composition options for modals
selfcontained Nov 29, 2022
9d01bcd
updating classnames dep and using ReactNode type
selfcontained Dec 1, 2022
44d3d87
cleanup and apply sub-components when title exists
selfcontained Dec 2, 2022
692cc8d
adjust to overflow-y-auto
selfcontained Dec 2, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion components/dashboard/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
"@stripe/react-stripe-js": "^1.7.2",
"@stripe/stripe-js": "^1.29.0",
"@types/react-datepicker": "^4.8.0",
"classnames": "^2.3.1",
"configcat-js": "^6.0.0",
"countries-list": "^2.6.1",
"dayjs": "^1.11.5",
Expand Down Expand Up @@ -43,7 +44,6 @@
"@typescript-eslint/eslint-plugin": "^4.21.0",
"@typescript-eslint/parser": "^4.21.0",
"autoprefixer": "^9.8.6",
"classnames": "^2.3.1",
"cypress": "^9.2.1",
"eslint": "^7.24.0",
"eslint-config-react-app": "^6.0.0",
Expand Down
60 changes: 43 additions & 17 deletions components/dashboard/src/components/Modal.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,8 @@
* See License-AGPL.txt in the project root for license information.
*/

import { useEffect } from "react";
import { ReactNode, useEffect } from "react";
import cn from "classnames";
selfcontained marked this conversation as resolved.
Show resolved Hide resolved
import { getGitpodService } from "../service/service";

type CloseModalManner = "esc" | "enter" | "x";
Expand All @@ -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;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

children: ReactNode;
visible: boolean;
closeable?: boolean;
className?: string;
Expand Down Expand Up @@ -75,10 +76,10 @@ export default function Modal(props: {
<div className="fixed top-0 left-0 bg-black bg-opacity-70 z-50 w-screen h-screen">
<div className="w-screen h-screen align-middle" style={{ display: "table-cell" }}>
<div
className={
"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 " +
(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 ",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

props.className,
)}
>
{props.closeable !== false && (
<div
Expand All @@ -93,16 +94,9 @@ export default function Modal(props: {
)}
{props.title ? (
<>
<h3 className="pb-2">{props.title}</h3>
<div
className={
"border-gray-200 dark:border-gray-800 -mx-6 px-6 " +
(props.hideDivider ? "" : "border-t border-b mt-2 py-4")
}
>
{props.children}
</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>
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

<ModalFooter>{props.buttons}</ModalFooter>
Comment on lines +97 to +99
Copy link
Contributor

Choose a reason for hiding this comment

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

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

</>
) : (
props.children
Expand All @@ -112,3 +106,35 @@ export default function Modal(props: {
</div>
);
}

type ModalHeaderProps = {
children: ReactNode;
};

export const ModalHeader = ({ children }: ModalHeaderProps) => {
return <h3 className="pb-2">{children}</h3>;
};

type ModalBodyProps = {
children: ReactNode;
hideDivider?: boolean;
};

export const ModalBody = ({ children, hideDivider = false }: ModalBodyProps) => {
return (
<div
className={cn("overflow-y-auto border-gray-200 dark:border-gray-800 -mx-6 px-6 ", {
"border-t border-b mt-2 py-4": !hideDivider,
})}
>
{children}
</div>
);
};

type ModalFooterProps = {
children: ReactNode;
};
export const ModalFooter = ({ children }: ModalFooterProps) => {
return <div className="flex justify-end mt-6 space-x-2">{children}</div>;
};
12 changes: 6 additions & 6 deletions components/dashboard/src/settings/EnvironmentVariables.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import { UserEnvVar, UserEnvVarValue } from "@gitpod/gitpod-protocol";
import { useEffect, useRef, useState } from "react";
import ConfirmationModal from "../components/ConfirmationModal";
import { Item, ItemField, ItemFieldContextMenu, ItemsList } from "../components/ItemsList";
import Modal from "../components/Modal";
import Modal, { ModalBody, ModalFooter, ModalHeader } from "../components/Modal";
import { getGitpodService } from "../service/service";
import { PageWithSettingsSubMenu } from "./PageWithSettingsSubMenu";

Expand Down Expand Up @@ -52,8 +52,8 @@ function AddEnvVarModal(p: EnvVarModalProps) {
return (
// TODO: Use title and buttons props
<Modal visible={true} onClose={p.onClose} onEnter={save}>
<h3 className="mb-4">{isNew ? "New" : "Edit"} Variable</h3>
<div className="border-t border-b border-gray-200 dark:border-gray-800 -mx-6 px-6 py-4 flex flex-col">
<ModalHeader>{isNew ? "New" : "Edit"} Variable</ModalHeader>
<ModalBody>
{error ? (
<div className="bg-gitpod-kumquat-light rounded-md p-3 text-gitpod-red text-sm mb-2">{error}</div>
) : null}
Expand Down Expand Up @@ -98,15 +98,15 @@ function AddEnvVarModal(p: EnvVarModalProps) {
make it available in more projects.
</p>
</div>
</div>
<div className="flex justify-end mt-6">
</ModalBody>
<ModalFooter>
<button className="secondary" onClick={p.onClose}>
Cancel
</button>
<button className="ml-2" onClick={save}>
{isNew ? "Add" : "Update"} Variable
</button>
</div>
</ModalFooter>
</Modal>
);
}
Expand Down
12 changes: 6 additions & 6 deletions components/dashboard/src/settings/Integrations.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import ConfirmationModal from "../components/ConfirmationModal";
import { ContextMenuEntry } from "../components/ContextMenu";
import InfoBox from "../components/InfoBox";
import { Item, ItemField, ItemFieldContextMenu, ItemFieldIcon, ItemsList } from "../components/ItemsList";
import Modal from "../components/Modal";
import Modal, { ModalBody, ModalHeader, ModalFooter } from "../components/Modal";
import copy from "../images/copy.svg";
import exclamation from "../images/exclamation.svg";
import { openAuthorizeWindow } from "../provider-utils";
Expand Down Expand Up @@ -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>
Copy link
Contributor

Choose a reason for hiding this comment

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

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>

<ModalBody>
<div className="text-gray-500">Configure provider permissions.</div>
{(editModal.provider.scopes || []).map((scope) => (
<div key={`scope-${scope}`}>
Expand All @@ -307,15 +307,15 @@ function GitProviders() {
></CheckBox>
</div>
))}
</div>
<div className="flex justify-end mt-6">
</ModalBody>
<ModalFooter>
<button
onClick={() => updatePermissions()}
disabled={equals(editModal.nextScopes, editModal.prevScopes)}
>
Update Permissions
</button>
</div>
</ModalFooter>
</Modal>
)}

Expand Down