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

ui v2: restyle dialog component #731

Merged
merged 5 commits into from
Dec 7, 2020
Merged

ui v2: restyle dialog component #731

merged 5 commits into from
Dec 7, 2020

Conversation

dschaller
Copy link
Contributor

@dschaller dschaller commented Dec 2, 2020

Description

restyle dialog

Screen Shot 2020-12-01 at 10 48 34 PM

Screen Shot 2020-12-01 at 10 48 38 PM

Testing Performed

storybook

Todo

  • hook up the close icon

@dschaller dschaller requested a review from a team as a code owner December 2, 2020 06:49
@dschaller dschaller marked this pull request as draft December 2, 2020 06:53
@dschaller dschaller changed the title ui v2: restyle modal component ui v2: restyle dialog component Dec 2, 2020
@dschaller dschaller marked this pull request as ready for review December 2, 2020 21:27
Comment on lines 16 to 18
fontSize: "16px",
padding: "16px 16px 0 16px",
fontWeight: 400,
Copy link
Collaborator

Choose a reason for hiding this comment

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

in Figma it's Roboto Medium 20px

Suggested change
fontSize: "16px",
padding: "16px 16px 0 16px",
fontWeight: 400,
fontSize: "20px",
fontWeight: 500,
padding: "16px 16px 0 16px"

fontSize: "16px",
padding: "16px 16px 0 16px",
fontWeight: 400,
color: "#000000",
Copy link
Collaborator

Choose a reason for hiding this comment

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

from figma

Suggested change
color: "#000000",
color: "#0d1030",

height: "16px",
width: "16px",
padding: "0",
color: "#000000",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
color: "#000000",
color: "#0d1030",


const DialogContent = styled(MuiDialogContent)({
padding: "8px 16px",
fontSize: "12px",
Copy link
Collaborator

Choose a reason for hiding this comment

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

from figma

Suggested change
fontSize: "12px",
fontSize: "16px",

frontend/packages/core/src/dialog.tsx Show resolved Hide resolved
content: string;
open: boolean;
children: React.ReactNode;
actions: React.ReactNode;
onClose: () => void;
Copy link
Collaborator

Choose a reason for hiding this comment

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

i'm not sure that onClose should ever do anything other than dismiss the dialog, that seems like a bad UX pattern to allow devs to tie behavior to onClose. any use case in mind?

);
};
export const Dialog = ({ title, children, open, onClose, actions }: DialogProps) => (
<MuiDialog open={open} onClose={onClose}>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Dialog has border: 1px solid rgba(13, 16, 48, 0.1); in Figma

and the box shadow doesn't match Figma's box-shadow: 0px 10px 24px rgba(35, 48, 143, 0.3);

content: string;
open: boolean;
children: React.ReactNode;
actions: React.ReactNode;
Copy link
Collaborator

Choose a reason for hiding this comment

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

we should standardize, either use these richer API where you pass components to be rendered nested vs just using children for everything and passing in Actions as part of the children (this same thing applies for accordion). i think we just need to some quick pro/con on it.

my main thought in potentially choosing the children-only API is that it reads more literally despite being a little more complicated for the user (but that's what stories are for, or maybe an invariant check on top of it), i.e. title > content > buttons, whereas this API reads title > buttons > content but renders as title > content > buttons

@@ -20,11 +20,13 @@ const BaseTextField = ({ InputProps, InputLabelProps, ...props }: MuiStandardTex
);

const StyledTextField = styled(BaseTextField)({
padding: "8px 0",
Copy link
Collaborator

Choose a reason for hiding this comment

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

another decision point here, should children have inherent padding or should the parent pad it's children (e.g. something like a & > * selector in containers), not sure what's best practice

Copy link
Collaborator

@danielhochman danielhochman left a comment

Choose a reason for hiding this comment

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

beautiful

@dschaller dschaller merged commit 5e2050c into UIV2 Dec 7, 2020
@dschaller dschaller deleted the modal branch December 7, 2020 23:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants