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

Change indent width UI #1818

Merged
merged 4 commits into from
Oct 12, 2012
Merged

Change indent width UI #1818

merged 4 commits into from
Oct 12, 2012

Conversation

jasonsanjose
Copy link
Member

Use Garth's updated statusbar Ui.

@ghost ghost assigned redmunds Oct 11, 2012
@redmunds
Copy link
Contributor

There's a tooltip over Tab Size/Spaces, but there's no tooltip over the number. The underscore during mouseover is so small for the number that it's possible to cover it up with the mouse pointer, so it seems like it worth adding.

@redmunds
Copy link
Contributor

Are there supposed to be spinner up/down arrows like in Garth's prototype? If so, I'm not seeing them. But, I discovered that up/down arrows work.

@jasonsanjose
Copy link
Member Author

The new prototype doesn't have spinners http://garthdb.github.com/Brackets-UI-Prototypes/status_bar/

function _changeIndentSize(inc) {
function _changeIndentWidth(value) {
$indentWidthLabel.toggleClass("hidden");
$indentWidthInput.toggleClass("hidden");
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like it's possible to get into the opposite state than what is desired with the hidden class because you assume it's applied and toggleClass() will remove it. Same comment for handling "click" event: it assumes hidden is not applied and toggleClass() will add it.

I think it will be safer to be more explicit about your intentions by using removeClass("hidden") here and addClass("hidden") when handling the "click" event.

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

@jasonsanjose
Copy link
Member Author

Good idea on the tooltip. I've added that in my last commit.

@@ -136,36 +135,50 @@ a, img {
font-size: 1.4em;
}

#status-indent div {
#status-indent * {
Copy link
Contributor

Choose a reason for hiding this comment

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

Using the universal selector as a descendent selector selects every element in the DOM sub-tree. Is that what you want? If you only meant to apply it to immediate children then use the child combinator:

#status-indent > *

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

@redmunds
Copy link
Contributor

Done with initial review

@jasonsanjose
Copy link
Member Author

@redmunds thanks! Fixes pushed.

@redmunds
Copy link
Contributor

Looks good. Merging.

# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants