Created attachment 326781 [details] [Image] Button with image/text Make ButtonNavigationItem more flexible. Currently it auto-detects the button style (image or text-only) at construction time, based on arguments passed to the constructor. It isn't possible to create a button with both an image and text. This patch adds a button style enum. Buttons continue to detect the correct style when created (Image or Text), but can also be set to ImageAndText: let item = new WI.ButtonNavigationItem("import", WI.UIString("Import"), "Images/Import.svg", 16, 16); item.buttonStyle = WI.ButtonNavigationItem.Style.ImageAndText;
<rdar://problem/35512238>
Interesting, I like it! There seems to be a bit of extra padding. ~26px for this one compared to ~13px for the icon only items.
Created attachment 326803 [details] [Image] Alignment across all button types Some tweaks were needed to get all three button types aligned properly. Icons should have same vertical alignment, whether they appear in image-only or text-and-image buttons.
(In reply to Joseph Pecoraro from comment #2) > Interesting, I like it! > > There seems to be a bit of extra padding. ~26px for this one compared to > ~13px for the icon only items. Hmm, will investigate.
Created attachment 326810 [details] Patch
(In reply to Matt Baker from comment #4) > (In reply to Joseph Pecoraro from comment #2) > > Interesting, I like it! > > > > There seems to be a bit of extra padding. ~26px for this one compared to > > ~13px for the icon only items. > > Hmm, will investigate. Image buttons have a 26px width. In this screenshot, the "refresh" button's icon is 13px wide, leaving 6.5px of padding on each side. Text buttons have 2px of left/right margin, a 1px border, and 8px of left/right padding, for a total of 11px of padding on each side. All the margin/padding/border styles are for scope/radio buttons, which use a background color for hover/activated effects.
If we want to support hover effects for all buttons (like the close button in the new Network details view), then we'd want to add margin/padding to image buttons as well and we could further unify button styles.
Comment on attachment 326810 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=326810&action=review r=me > Source/WebInspectorUI/UserInterface/Views/ButtonNavigationItem.js:131 > + for (let key in WI.ButtonNavigationItem.Style) > + this.element.classList.remove(WI.ButtonNavigationItem.Style[key]); You could write this in one line. this.element.classList.remove(...Object.values(WI.ButtonNavigationItem.Style)); > Source/WebInspectorUI/UserInterface/Views/ButtonNavigationItem.js:171 > + this._labelElement = this.element.appendChild(document.createElement("span")); I realize that `_update()` isn't called that often, but it would be nice if we didn't have to create a new element each time. Also, it seems like you don't even really need to save this to a member variable, so I'd either rename it to a local variable `labelElement` or add a check to make sure it isn't recreated with each `_update()`. > Source/WebInspectorUI/UserInterface/Views/CheckboxNavigationItem.js:73 > + Checked: "checkbox-navigation-item-checked", This name implies it will only be fired when the checkbox is checked, not when it is unchecked. I'd rename it to "Update" to match the native event name. > Source/WebInspectorUI/UserInterface/Views/NavigationBar.css:61 > +.navigation-bar .item.checkbox { > + padding: 1px 8px 3px; > +} > + > +.navigation-bar .item.checkbox label { > + -webkit-padding-start: 3px; > +} We should move this to its own CheckboxNavigationItem.css file. I find it confusing when I'm looking for the styles of a particular class and there is no corresponding *.css file, and instead everything is in some parent file.
Comment on attachment 326810 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=326810&action=review >> Source/WebInspectorUI/UserInterface/Views/ButtonNavigationItem.js:171 >> + this._labelElement = this.element.appendChild(document.createElement("span")); > > I realize that `_update()` isn't called that often, but it would be nice if we didn't have to create a new element each time. Also, it seems like you don't even really need to save this to a member variable, so I'd either rename it to a local variable `labelElement` or add a check to make sure it isn't recreated with each `_update()`. I'll make labelElement local and ditch the member variable. I think it's okay to recreate it each time, since _update isn't called often. >> Source/WebInspectorUI/UserInterface/Views/CheckboxNavigationItem.js:73 >> + Checked: "checkbox-navigation-item-checked", > > This name implies it will only be fired when the checkbox is checked, not when it is unchecked. I'd rename it to "Update" to match the native event name. What do you think about CheckedDidChange? >> Source/WebInspectorUI/UserInterface/Views/NavigationBar.css:61 >> +} > > We should move this to its own CheckboxNavigationItem.css file. I find it confusing when I'm looking for the styles of a particular class and there is no corresponding *.css file, and instead everything is in some parent file. Sometimes I'm hesitant to add tiny CSS files, but I agree it can be confusing. Will add.
Created attachment 327140 [details] Patch
Comment on attachment 327140 [details] Patch r=me
Comment on attachment 327140 [details] Patch Clearing flags on attachment: 327140 Committed r224978: <https://trac.webkit.org/changeset/224978>
All reviewed patches have been landed. Closing bug.
Hmm, when the button is disabled the icon grays out but not the text. Do you think the text should gray out a little?
(In reply to Joseph Pecoraro from comment #14) > Hmm, when the button is disabled the icon grays out but not the text. Do you > think the text should gray out a little? It definitely should! Will fix.