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

@uppy/dashboard - made added-files previews look more proportional #1588

Merged
merged 5 commits into from
May 24, 2019

Conversation

lakesare
Copy link
Collaborator

Was:
image

Now:
image

@lakesare
Copy link
Collaborator Author

@arturi, do you think that's enough? (That's the widest they'll be)

@@ -707,7 +707,7 @@ a.uppy-Dashboard-poweredBy {

.uppy-size--md & {
width: 100%;
height: 100px;
height: 129px;
border: 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

129px vs 130px? I’m guessing there’s a reason, that’s why I’m asking, so I can keep that in the back of my head for future refactors.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Exactly, 129px is intentional, in that it's exactly the height that makes 1-line filenames and 2-line filenames not affect the height of the image. Here is an exaggerated example of height>129px:
image

@arturi
Copy link
Contributor

arturi commented May 23, 2019

Thank you!

What about:

  1. Removing height from .uppy-DashboardItem (for all sizes).
  2. Setting height in .uppy-size--md .uppy-DashboardItem-preview to 135px.

Just tried this and it seems to work great. Maybe I missed something?

@lakesare
Copy link
Collaborator Author

lakesare commented May 23, 2019

We need the height set, otherwise flex won't be stacking up images well: image.

@lakesare
Copy link
Collaborator Author

Hold on, I'll make it less wide.

@lakesare
Copy link
Collaborator Author

Widest now: image

@arturi
Copy link
Contributor

arturi commented May 23, 2019

Nice! I like the sizes 👍 Thanks for updating.

Noticed this in Firefox 🙀:

Screen Shot 2019-05-23 at 19 30 38

Thinking if we can make the height of the of the preview fixed, so it doesn’t change depending on the height of the filename line? It shrinks because of flex, right?

@lakesare
Copy link
Collaborator Author

lakesare commented May 23, 2019

It shrinks because of flex, right?

Right.
Can't reproduce this issue in firefox, but I added flex-shrink: 0 to the image, this should compensate for tiny differences in font, or what was causing that glitch in firefox.

Here is the widest screen now:
image

@arturi
Copy link
Contributor

arturi commented May 24, 2019

Nice! Thanks.

Made a small suggestion tweak, to go from this height:

Screen Shot 2019-05-24 at 12 43 09

To this:

Screen Shot 2019-05-24 at 12 43 52

And make the numbers even, but again, maybe I’ve missed something and it breaks the layout? In my tests it was fine.

Co-Authored-By: Artur Paikin <artur@arturpaikin.com>
@lakesare
Copy link
Collaborator Author

lakesare commented May 24, 2019

@arturi, it's fine in my tests too, looks better to me.

@arturi
Copy link
Contributor

arturi commented May 24, 2019

Ok, cool. There’s another suggestion for height: 152px --> height: 140px that didn’t make it into you commit, did you mean you tested with that?

Co-Authored-By: Artur Paikin <artur@arturpaikin.com>
@lakesare
Copy link
Collaborator Author

Oh no, pulled that change too now. Tested it with firefox, safari and chrome, - everything looks good.

@arturi arturi merged commit d847270 into master May 24, 2019
@arturi arturi deleted the fix/sizing-for-added-files branch May 24, 2019 11:47
@arturi
Copy link
Contributor

arturi commented May 24, 2019

Awesome, merged! Thanks.

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