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

Design changes #781

Merged
merged 12 commits into from
Feb 24, 2021
Merged

Design changes #781

merged 12 commits into from
Feb 24, 2021

Conversation

Tbaut
Copy link
Collaborator

@Tbaut Tbaut commented Feb 19, 2021

closes #776

image

@render
Copy link

render bot commented Feb 19, 2021

@render
Copy link

render bot commented Feb 19, 2021

@render
Copy link

render bot commented Feb 19, 2021

@render
Copy link

render bot commented Feb 19, 2021

@render
Copy link

render bot commented Feb 19, 2021

A deploy for your Render PR Server at https://components-storybook-dev-pr-781.onrender.com just failed.

View details on your dashboard at https://dashboard.render.com/static/srv-c0nspjjjbvmciotr8q7g.

@render
Copy link

render bot commented Feb 19, 2021

Your Render PR Server at https://components-storybook-dev-pr-781.onrender.com is now live!

View it on your dashboard at https://dashboard.render.com/static/srv-c0nspjjjbvmciotr8q7g.

@tanmoyAtb
Copy link
Contributor

closes #776

What I would like to get some help/guidance on:

1. I feel the added css is hacky, (see mention in Github comments)

2. I guess I haven't figured out the correct way to use `palette.additional["gray"][8]`, the "Add more files" should have a light gray background  [according to the design](https://user-images.githubusercontent.com/33178835/108517161-5a102d00-72c7-11eb-9004-4390e75c035a.png).

3. [according to the design](https://user-images.githubusercontent.com/33178835/108517161-5a102d00-72c7-11eb-9004-4390e75c035a.png) the "done" button is large. Do we have any other such buttons in the UI? using a `variant="large"` made it wider, and larger. Wondering if I missed something, if I should leave it as is, or if we should create some new variant "wide" or something.

4. This red "Please select a file" at the end my demo isn't super nice either :)

Here's the upload flow:
upload.mp4

hey @Tbaut
I placed some replies to your comments, hoping they would help with 1 & 2. for -
3. For the button, I would simply add a width to the button under a CSS className, not add a variant.
4. Yes the Please select a file doesn't look right. centralizing it might improve it, or we can always ask cindy
to put up a design quickly

@Tbaut
Copy link
Collaborator Author

Tbaut commented Feb 22, 2021

Thanks a lot @tanmoyAtb your guidance helped a lot.
I pushed some changes, and try to not have this PR go in too many directions.
I will comment in github why I did some things I did.

@render
Copy link

render bot commented Feb 22, 2021

Your Render PR Server at https://components-storybook-dev-pr-781.onrender.com is now live!

View it on your dashboard at https://dashboard.render.com/static/srv-c0nspjjjbvmciotr8q7g.

const classes = useStyles()
const { uploadFiles, currentPath } = useDrive()

const UploadSchema = object().shape({
files: array()
.min(1, "Please select a file")
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

With the "Done" button being disabled when there's no file to upload, it gives the nice UX that users understand right away that they can't upload anything, and we don't need to have this error message (that we discussed earlier wasn't nicely placed).

@render
Copy link

render bot commented Feb 22, 2021

Your Render PR Server at https://components-storybook-dev-pr-781.onrender.com is now live!

View it on your dashboard at https://dashboard.render.com/static/srv-c0nspjjjbvmciotr8q7g.

Copy link
Contributor

@tanmoyAtb tanmoyAtb left a comment

Choose a reason for hiding this comment

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

Looks great !
Some pointers -
The space above the list of files should be smaller !
Screenshot 2021-02-22 at 11 26 32 PM

className={clsx(classes.okButton, "wide")}
disabled={isDoneDisabled}
>
<Trans>Done</Trans>
Copy link
Contributor

Choose a reason for hiding this comment

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

should be Upload

Copy link
Collaborator Author

@Tbaut Tbaut Feb 22, 2021

Choose a reason for hiding this comment

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

Thanks Tanmoy, I've discussed this with @sweetpea22 and the "Upload" had been understood by user as a "Add more", or "Browse". That's why we thought the button should rather give the confidence to users that it should be clicked when they are done. Maybe "Submit" could be better?

Copy link
Contributor

Choose a reason for hiding this comment

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

How about "Start Upload"?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes "Start upload" seems more relatable to me !

Copy link
Contributor

Choose a reason for hiding this comment

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

okay, should we change txt content of the first upload modal (before adding files) to "start upload" too or nah?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think the text is fine, you're selecting files. The real upload would happen when you "strat upload" :)

Changing that.

@Tbaut
Copy link
Collaborator Author

Tbaut commented Feb 22, 2021

Here's what it looks like now:

edit: the space between the top of the modal and the "Add more files" are still inconsistent. Making this space smaller looks weird to me though.

Should I make it look like this? :
edit: did it.
image

This is how it looks with the last commit
https://user-images.githubusercontent.com/33178835/108748326-bdf05b00-753e-11eb-83b2-83be9b6bce02.mp4

@render
Copy link

render bot commented Feb 22, 2021

Your Render PR Server at https://components-storybook-dev-pr-781.onrender.com is now live!

View it on your dashboard at https://dashboard.render.com/static/srv-c0nspjjjbvmciotr8q7g.

@Tbaut
Copy link
Collaborator Author

Tbaut commented Feb 23, 2021

I addressed the spacing issue.

@render
Copy link

render bot commented Feb 23, 2021

Your Render PR Server at https://components-storybook-dev-pr-781.onrender.com is now live!

View it on your dashboard at https://dashboard.render.com/static/srv-c0nspjjjbvmciotr8q7g.

@render
Copy link

render bot commented Feb 23, 2021

Your Render PR Server at https://components-storybook-dev-pr-781.onrender.com is now live!

View it on your dashboard at https://dashboard.render.com/static/srv-c0nspjjjbvmciotr8q7g.

Copy link
Contributor

@tanmoyAtb tanmoyAtb left a comment

Choose a reason for hiding this comment

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

Looks good !

Copy link
Contributor

@sweetpea22 sweetpea22 left a comment

Choose a reason for hiding this comment

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

hey guys, the background of modals has changed to gray[2], easier on the eyes 😛
prob can address this in another PR.

Copy link
Contributor

@FSM1 FSM1 left a comment

Choose a reason for hiding this comment

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

great work

@render
Copy link

render bot commented Feb 23, 2021

Your Render PR Server at https://components-storybook-dev-pr-781.onrender.com is now live!

View it on your dashboard at https://dashboard.render.com/static/srv-c0nspjjjbvmciotr8q7g.

@render
Copy link

render bot commented Feb 23, 2021

Your Render PR Server at https://components-storybook-dev-pr-781.onrender.com is now live!

View it on your dashboard at https://dashboard.render.com/static/srv-c0nspjjjbvmciotr8q7g.

@Tbaut
Copy link
Collaborator Author

Tbaut commented Feb 24, 2021

Thanks for the help @FSM1. I'll open a follow-up PR for the modal @sweetpea22, and the translation in a pre-commit so that we don't forget about it.

@Tbaut Tbaut merged commit feffe35 into dev Feb 24, 2021
@Tbaut Tbaut deleted the fix/tbaut-design-change-776 branch February 24, 2021 10:34
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.

Design Changes
4 participants