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

Add ellipsis to overflowing layer item name #559 #577

Merged
merged 4 commits into from
Dec 6, 2016

Conversation

GMartigny
Copy link
Contributor

@GMartigny GMartigny commented Dec 1, 2016

If I understand well your last comment on the issue, you want a tooltip when the layer name is overflowing ?

It's going to be hard to have tooltip only in case of overflowing (not impossible though) and lost native CSS performance.


This change is Reviewable

@juliandescottes
Copy link
Collaborator

If I understand well your last comment on the issue, you want a tooltip when the layer name is overflowing

I don't think we need the usual jquery tooltip here. Simply set the title attribute on the layer element to have the default HTML tooltip displayed after some time. No need to make it depend on the overflow either.

@@ -36,7 +36,7 @@ <h3 class="toolbox-title layers-title">Layers
<script type="text/template" id="layer-item-template">
<li class="layer-item {{isselected:current-layer-item}}"
data-layer-index="{{layerindex}}">
{{layername}}
<span class="layer-name">{{layername}}</span>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Everything looks good! let's just add title="{{layername}}" here and it should be good to go :)

Thanks!

@GMartigny
Copy link
Contributor Author

Sorry for the spam 🙄, but it can be JQuery tooltip (init on the fly).

I see that it's never done in the code base. Is that on purpose or it just never had the need ?

@juliandescottes
Copy link
Collaborator

I think we never had to dynamically add / remove tooltips yet.

In this case I'm just leaning towards the default HTML tooltip, because I want it to be less intrusive than the jquery one. At least I would prefer if it was not displayed immediately (since it's a redundant information in case there is no ellipsis).

If you have a solution in mind using the jquery tooltip, feel free to update the PR, I'll test it and give you my feedback.

Copy link
Collaborator

@juliandescottes juliandescottes left a comment

Choose a reason for hiding this comment

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

Thanks for the update! Can you address my two comments ? I'll merge right after.

if (layerItem.offsetWidth < layerItem.scrollWidth) {
$(layerItem).find('.layer-name')
.addClass('overflowing-name')
.attr('title', pskl.utils.TooltipFormatter.format(layer.getName()))
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think you need the TooltipFormatter here, it is only used for complex tooltips.

@@ -70,6 +70,8 @@
padding: 0 0 0 10px;
flex: 1 auto;
white-space: nowrap;
}
.layer-item .layer-name.overflowing-name {
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: let's add en empty line before this new selector.

@juliandescottes
Copy link
Collaborator

Thanks, merging!

@juliandescottes juliandescottes merged commit 732c3c2 into piskelapp:master Dec 6, 2016
@GMartigny GMartigny deleted the 559-TooLongLayerName branch December 8, 2016 15:26
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