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

Fixes for new dashboard texts #2499

Merged
merged 12 commits into from
Feb 26, 2015
Merged

Fixes for new dashboard texts #2499

merged 12 commits into from
Feb 26, 2015

Conversation

viddo
Copy link
Contributor

@viddo viddo commented Feb 25, 2015

description with a larger characters: screen shot 2015-02-25 at 16 14 38
description with a smaller characters: screen shot 2015-02-25 at 16 15 33



@viddo
Copy link
Contributor Author

viddo commented Feb 25, 2015

@saleiva @xavijam please review.

@xavijam
Copy link
Contributor

xavijam commented Feb 25, 2015

Only texts? In that case, not much to say, 👍.

@@ -7,7 +7,7 @@ var LikesView = require('../../new_common/views/likes/view');
var Utils = require('cdb.Utils');
cdb.admin = require('cdb.admin');

var SHORT_DES = 85;
var SHORT_DES = 45;
Copy link
Contributor

Choose a reason for hiding this comment

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

No css solution?

@saleiva
Copy link
Contributor

saleiva commented Feb 25, 2015

👍

@xavijam
Copy link
Contributor

xavijam commented Feb 25, 2015

IMHO 45 characters for map description is too short.

@viddo
Copy link
Contributor Author

viddo commented Feb 25, 2015

Yes, it's short, and as I wrote on the related issue it's sub-optimal but there is no perfect solution. The CSS approach works perfect on webkit, but for the rest there would be no ellipsis (or other indicator of more text). If you're fine with the following scenario I'm happy to change it to a CSS solution:

  • webkit: screen shot 2015-02-25 at 14 52 00
  • non-webkit: screen shot 2015-02-25 at 14 51 49

@xavijam
Copy link
Contributor

xavijam commented Feb 25, 2015

How about to adding a white gradient at the end?
For the rest of the browsers, i mean. cc @piensaenpixel

@piensaenpixel
Copy link
Contributor

+1 @xavijam love your comment

@viddo
Copy link
Contributor Author

viddo commented Feb 25, 2015

Ah yes, you're right that should do. For the sake of simplicity I would leave the gradient since it's so subtle anyway. Looks OK?

screen shot 2015-02-25 at 16 56 52
screen shot 2015-02-25 at 16 57 11

Use gradient to indicate more text on non-webkit browsers, since other
non-webkit browsers don’t support multiline text-overflow ellipsis.
@viddo
Copy link
Contributor Author

viddo commented Feb 25, 2015

retest this please

@piensaenpixel
Copy link
Contributor

@viddo one question, what do you think about remove the gradient for non-webkit browsers? really if we can show the ellipsis, i think we dont need the gradient

@viddo
Copy link
Contributor Author

viddo commented Feb 25, 2015

But the ellipsis is only available for webkit, so I suggest to leave the gradient.

@piensaenpixel
Copy link
Contributor

Sorry... it was my fault. I mean remove the gradient for webkit browsers ;)

@piensaenpixel
Copy link
Contributor

and one more thing did you test this when you selected the map? you should change the color of the gradient. :)

Since only applied for MapCard anyway, and need to know the colors to
be able to match hover/selected states. Changed transparency to rgba
values too to work on firefox
@viddo
Copy link
Contributor Author

viddo commented Feb 25, 2015

@piensaenpixel: Fixed the hover/selected states (good catch!) in the last commit ffb1855.
However I do not see any issue way to opt-out of the gradient for webkit only. That's why I suggested to leave it, to not complicate things by special rules and hacks.

Nicklas Gummesson added 2 commits February 25, 2015 18:42
Container must have a width for the text-overflow to work
@viddo
Copy link
Contributor Author

viddo commented Feb 25, 2015

@xavijam fixed privacy indicator in FF as well as the map card titles that was not ellipsed properly there either, see last two commits.

@xavijam
Copy link
Contributor

xavijam commented Feb 25, 2015

🇺🇸

@Cartofante
Copy link
Collaborator

Frontend tests were OK 👍 (details)

@piensaenpixel
Copy link
Contributor

🇺🇸

viddo added a commit that referenced this pull request Feb 26, 2015
@viddo viddo merged commit e7da36f into master Feb 26, 2015
@viddo viddo deleted the fix-new-dashboard-copy branch February 26, 2015 09:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants