By default, only show text labels in the toolbar, but add a settings option to show the icons.
Created attachment 161880 [details] Patch
Attachment 161880 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/ChangeLog:11: Line contains tab character. [whitespace/tab] [5] Total errors found: 1 in 6 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 161880 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=161880&action=review > Source/WebCore/ChangeLog:6 > + Reviewed by NOBODY (OOPS!). Please attach screenshots with changes to the UI. > Source/WebCore/inspector/front-end/Settings.js:47 > + showUseToolbarIcons: true I don't think we need this. > Source/WebCore/inspector/front-end/SettingsScreen.js:254 > + p.appendChild(this._createCheckboxSetting(WebInspector.UIString("Show toolbar icons"), WebInspector.settings.useToolbarIcons)); Strings should go to the WebCore/English.lproj/localizedStrings.js > Source/WebCore/inspector/front-end/Toolbar.js:167 > + _useToolbarIconsSettingChanged: function(event) { { on the next line. > Source/WebCore/inspector/front-end/Toolbar.js:171 > + _setIconsVisible: function(visible) { ditto > Source/WebCore/inspector/front-end/Toolbar.js:249 > + if (toolbarItems[i].offsetTop > 1) Should we compare things with 1 pixel? What is the reason behind this change? > Source/WebCore/inspector/front-end/inspector.css:68 > +#toolbar.with-icons + #main { We don't use sibling selectors. > Source/WebCore/inspector/front-end/inspector.css:99 > +body.compact #toolbar.with-icons + #main { ditto > Source/WebCore/inspector/front-end/inspector.css:208 > + /* A line height of 0 allows precise text positioning using padding. */ I was hoping that this change would be simpler. Is there a way to optimize the styles here?
Created attachment 161881 [details] Screenshot showing the new UI in the different states
(In reply to comment #3) > (From update of attachment 161880 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=161880&action=review > > > Source/WebCore/ChangeLog:6 > > + Reviewed by NOBODY (OOPS!). > > Please attach screenshots with changes to the UI. Done. > > Source/WebCore/inspector/front-end/Settings.js:47 > > + showUseToolbarIcons: true > > I don't think we need this. Ok, I'll remove it. > > Source/WebCore/inspector/front-end/SettingsScreen.js:254 > > + p.appendChild(this._createCheckboxSetting(WebInspector.UIString("Show toolbar icons"), WebInspector.settings.useToolbarIcons)); > > Strings should go to the WebCore/English.lproj/localizedStrings.js > > > Source/WebCore/inspector/front-end/Toolbar.js:167 > > + _useToolbarIconsSettingChanged: function(event) { > > { on the next line. > > > Source/WebCore/inspector/front-end/Toolbar.js:171 > > + _setIconsVisible: function(visible) { > > ditto > > > Source/WebCore/inspector/front-end/Toolbar.js:249 > > + if (toolbarItems[i].offsetTop > 1) > > Should we compare things with 1 pixel? What is the reason behind this change? Since the body now has a 1px border when attached, the toolbar items will all have an offsetTop of 1. > > Source/WebCore/inspector/front-end/inspector.css:68 > > +#toolbar.with-icons + #main { > > We don't use sibling selectors. > > > Source/WebCore/inspector/front-end/inspector.css:99 > > +body.compact #toolbar.with-icons + #main { > > ditto > > > Source/WebCore/inspector/front-end/inspector.css:208 > > + /* A line height of 0 allows precise text positioning using padding. */ > > I was hoping that this change would be simpler. Is there a way to optimize the styles here? In the current UI, there are several places where things are not centered. I decided to fix those in addition to removing the toolbar icons. Would it be better to separate it into two patches?
> In the current UI, there are several places where things are not centered. I decided to fix those in addition to removing the toolbar icons. Would it be better to separate it into two patches? Yes please! It actually confused me...
Created attachment 178359 [details] [Patch] Rebaselined, made the mode opt-in, removed compact mode
Comment on attachment 178359 [details] [Patch] Rebaselined, made the mode opt-in, removed compact mode View in context: https://bugs.webkit.org/attachment.cgi?id=178359&action=review > Source/WebCore/ChangeLog:8 > + - removes compact mode in favor or dock-to-bottom of
Committed r137133: <http://trac.webkit.org/changeset/137133>
(In reply to comment #0) > By default, only show text labels in the toolbar, but add a settings option to show the icons. Should it be enabled by default? I updated to TOT and to my surprise things started looking differently. I had to trace back from the trac.webkit.org to find this change. Only when I stumbled upon here I came to know that there exists the option to change this behavior. I am sure many developers will face this. Just my thoughts.
> Should it be enabled by default? I updated to TOT and to my surprise things started looking differently. I had to trace back from the trac.webkit.org to find this change. Only when I stumbled upon here I came to know that there exists the option to change this behavior. I am sure many developers will face this. Just my thoughts. Yes, it should. Sorry for making it look confusing in the changelog - was a last minute decision. Iconless looks is where we headed long term, so I thought that making it default would be ok.