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

improvement: Make dialogs look more like dialogs #2686

Merged
merged 11 commits into from
Jan 3, 2021
Merged

improvement: Make dialogs look more like dialogs #2686

merged 11 commits into from
Jan 3, 2021

Conversation

lipis
Copy link
Member

@lipis lipis commented Dec 27, 2020

Preview: https://excalidraw-git-modal.excalidraw.now.sh

Before

Screenshot 2020-12-29 at 4 10 19 PM

After

Screenshot 2020-12-29 at 4 11 23 PM


TODO

  • Border
  • Border radius
  • Box shadow
  • Center aligned
  • Small Caps
  • Title background
  • Mobile view
  • Close button
  • Dark mode

@vercel
Copy link

vercel bot commented Dec 27, 2020

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/excalidraw/excalidraw/daco4s4a6
✅ Preview: https://excalidraw-git-modal.excalidraw.vercel.app

@lipis lipis requested a review from j-f1 December 27, 2020 22:35
@lipis lipis marked this pull request as draft December 28, 2020 11:51
@dwelle
Copy link
Member

dwelle commented Dec 28, 2020

I'd maybe add it to Islands in general? E.g. the Stats island could use one.

Btw, why do we add overflow-y: scroll to modals by default? Kinda ugly:

image

image

@lipis
Copy link
Member Author

lipis commented Dec 28, 2020

I'd maybe add it to Islands in general? E.g. the Stats island could use one.

Let's experiment with Islands in another PR... they have a shadow so they seem fine.

Btw, why do we add overflow-y: scroll to modals by default? Kinda ugly:

Removed the scroll..

Co-authored-by: Jed Fox <git@jedfox.com>
@dwelle
Copy link
Member

dwelle commented Dec 28, 2020

Trying out box-shadow instead of border (as discussed in chat):

border:
image

shadow:

image

If the shadow is too big, we can decrease. IMO looks more discrete.

Anyway, the border was also causing some overflow/scrollbar issue on body for smaller viewports (we'd have to investigate).

@lipis lipis changed the title feat: Add subtle border to modals feat: Make dialogs look more like dialogs Dec 29, 2020
@j-f1
Copy link
Member

j-f1 commented Dec 29, 2020

Needs some tweaks on mobile

@dwelle
Copy link
Member

dwelle commented Dec 29, 2020

Let's drop the title background. It adds too many visual boxes. Without it looks better and cleaner:

image

image

@lipis
Copy link
Member Author

lipis commented Dec 29, 2020

Updated.

@j-f1
Copy link
Member

j-f1 commented Dec 29, 2020

Why the small caps for the title? I feel like the design on master for the title is mostly OK already (except maybe for centering the title)

@lipis
Copy link
Member Author

lipis commented Dec 29, 2020

Why the small caps for the title? I feel like the design on master for the title is mostly OK already (except maybe for centering the title)

It made more sense with the title background.. and it was too big (h2).. but still I kind of like it to be a bit different from the rest of the dialog.

@dwelle
Copy link
Member

dwelle commented Dec 29, 2020

I have a strange fetish for small caps titles as well, but what do I know.

@lipis btw, I meant for the box-shadow to replace the border. IMO the border is too strong and doesn't add much value on top of the shadow.

@lipis
Copy link
Member Author

lipis commented Dec 29, 2020

@lipis btw, I meant for the box-shadow to replace the border. IMO the border is too strong and doesn't add much value on top of the shadow.

Yeah, I know.. I think it makes it sharper and the shadow is not that visible in the dark mode.. I tweaked the colors are bit.

Copy link
Member

@ad1992 ad1992 left a comment

Choose a reason for hiding this comment

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

Suggestion: I think this should be an improvement instead of a feature as it makes the existing feature better.

@lipis lipis changed the title feat: Make dialogs look more like dialogs improvement: Make dialogs look more like dialogs Dec 30, 2020
@dwelle
Copy link
Member

dwelle commented Jan 3, 2021

Ok, looks good!

@dwelle dwelle merged commit 6987816 into master Jan 3, 2021
@dwelle dwelle deleted the modal branch January 3, 2021 09:50
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.

None yet

4 participants