Bug 147578 – Web Inspector: Add Visual editors for CSS properties with comma separated values

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Devin Rousso]

Add tree outline style list editors for CSS properties with comma separated values.  Will be use in the new Visual style details panel in the CSS sidebar.

[Devin Rousso]

Created attachment 258094 [details]
Patch

[Devin Rousso]

Created attachment 258332 [details]
Patch

[Devin Rousso]

Created attachment 258462 [details]
Patch

[Timothy Hatcher]

Comment on attachment 258462 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=258462&action=review

> Source/WebInspectorUI/UserInterface/Images/Minus.svg:2
> +<!-- Copyright © 2014 Apple Inc. All rights reserved. -->

2015

> Source/WebInspectorUI/UserInterface/Views/VisualStyleCommaSeparatedKeywordEditor.js:124
> +    _onTreeItemSelect(item, selectedByUser)

No on prefix. Just _treeElementSelected. Also why item and not treeElement? It would tell you more what it is.

> Source/WebInspectorUI/UserInterface/Views/VisualStyleCommaSeparatedKeywordEditor.js:137
> +        let newItem = this._addCommaSeparatedKeyword(null, this._commaSeparatedKeywords.selectedTreeElementIndex);

newTreeElement?

> Source/WebInspectorUI/UserInterface/Views/VisualStyleCommaSeparatedKeywordEditor.js:152
> +        for (let treeItem of this._commaSeparatedKeywords.children) {

treeElement

> Source/WebInspectorUI/UserInterface/Views/VisualStyleCommaSeparatedKeywordEditor.js:167
> +        if (indexIsSet) {
> +            this._commaSeparatedKeywords.insertChild(valueElement, indexIsSet ? index + !this._insertNewItemsBeforeSelected : 0);
> +        } else

Nit: No need for the braces.

> Source/WebInspectorUI/UserInterface/Views/VisualStyleCommaSeparatedKeywordEditor.js:175
> +        if (!treeElement)

See!

> Source/WebInspectorUI/UserInterface/Views/VisualStyleCommaSeparatedKeywordEditor.js:185
> +    _createNewListItem(value)

_createNewTreeElement

> Source/WebInspectorUI/UserInterface/Views/VisualStyleFontFamilyListEditor.js:92
> +    _itemEditorKeyDown(event)

_treeElementKeyDown

> Source/WebInspectorUI/UserInterface/Views/VisualStyleFontFamilyListEditor.js:136
> +        let treeItem = this._commaSeparatedKeywords.selectedTreeElement;

treeElement

> Source/WebInspectorUI/UserInterface/Views/VisualStyleFontFamilyTreeItem.js:26
> +WebInspector.VisualStyleFontFamilyTreeItem = class VisualStyleFontFamilyTreeItem extends WebInspector.GeneralTreeElement

VisualStyleFontFamilyTreeElement. Classes should always end in a common suffix as what they subclass.

> Source/WebInspectorUI/UserInterface/Views/VisualStyleFontFamilyTreeItem.js:34
> +        this._keywordEditor.placeholder = "(" + WebInspector.UIString("modify the boxes below to add a value") + ")";

The () should be part of the UIString. I am also not sure the () are needed. Sentence case with a period might be best. Is a placeholder even needed?

[Devin Rousso]

(In reply to comment #4)
> > Source/WebInspectorUI/UserInterface/Views/VisualStyleFontFamilyTreeItem.js:34
> > +        this._keywordEditor.placeholder = "(" + WebInspector.UIString("modify the boxes below to add a value") + ")";
> 
> The () should be part of the UIString. I am also not sure the () are needed.
> Sentence case with a period might be best. Is a placeholder even needed?

Not having a placeholder made the empty rows appear very odd when the list is empty, as it would appear as a blue row with nothing in it.  I added the parenthesis to make it clear that the text was not meant to be the actual value and more like a hint, but I can remove it.

[Timothy Hatcher]

(In reply to comment #5)
> (In reply to comment #4)
> > > Source/WebInspectorUI/UserInterface/Views/VisualStyleFontFamilyTreeItem.js:34
> > > +        this._keywordEditor.placeholder = "(" + WebInspector.UIString("modify the boxes below to add a value") + ")";
> > 
> > The () should be part of the UIString. I am also not sure the () are needed.
> > Sentence case with a period might be best. Is a placeholder even needed?
> 
> Not having a placeholder made the empty rows appear very odd when the list
> is empty, as it would appear as a blue row with nothing in it.  I added the
> parenthesis to make it clear that the text was not meant to be the actual
> value and more like a hint, but I can remove it.

Okay, I think the parenthesis is fine. It should just be localized. Not all locales use the same parenthesis.

[Devin Rousso]

Created attachment 258991 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/22283812>

[Timothy Hatcher]

Comment on attachment 258991 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=258991&action=review

> Source/WebInspectorUI/UserInterface/Views/VisualStyleCommaSeparatedKeywordEditor.css:115
> +    filter: opacity(0.7);

opacity: 0.7;

[Devin Rousso]

Created attachment 259019 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 259019 [details]
Patch

Clearing flags on attachment: 259019

Committed r188483: <http://trac.webkit.org/changeset/188483>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.