Skip to content
This repository has been archived by the owner on Sep 6, 2021. It is now read-only.

Quick Edit tweak #5230

Merged
merged 6 commits into from
Sep 24, 2013
Merged

Quick Edit tweak #5230

merged 6 commits into from
Sep 24, 2013

Conversation

larz0
Copy link
Member

@larz0 larz0 commented Sep 16, 2013

listItem.attr("title", text);
var text = range.name + " <span class='related-file'>— " + range.textRange.document.file.name + " : " + (range.textRange.startLine + 1) + "</span>";
listItem.html(text);
listItem.attr("title", range.name + " " + range.textRange.document.file.name + " : " + (range.textRange.startLine + 1));
Copy link
Member

Choose a reason for hiding this comment

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

You might be able to replace this with listItem.attr(listItem.text())... could you try it out locally and see if that gives the right tooltip?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks Peter, it's so much better!

@peterflynn
Copy link
Member

@larz0 Looks good other than those two comments!

@ghost ghost assigned njx Sep 17, 2013
@julianasuh
Copy link
Contributor

Assigned to @njx

@njx
Copy link

njx commented Sep 23, 2013

@larz0 Looks great - just my one additional comment.

@njx
Copy link

njx commented Sep 24, 2013

@larz0 here's the comment I made - it's hidden right now because it was on one of the outdated diffs:

I think we might need to escape range.name too in _updateRangeLabel(). It happens that it's not necessary for CSS selectors (although it almost is, with >) and JS methods, but in the future if we had, say, LESS support, then there would be things like & in there, and there are other things people could use MultiRangeInlineEditor for that might have syntax that would need to be escaped.

@larz0
Copy link
Member Author

larz0 commented Sep 24, 2013

@njx fixed.

@njx
Copy link

njx commented Sep 24, 2013

Looks good. It turns out this broke one of the unit tests due to the string change, so I pushed up a fix for that as well. Merging.

njx pushed a commit that referenced this pull request Sep 24, 2013
@njx njx merged commit 844931f into master Sep 24, 2013
@njx njx deleted the larz/quick-edit-tweak branch September 24, 2013 18:00
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants