Bug 179100 - Web Inspector: Styles Redesign: turn on CSS spreadsheet editor by default
Summary: Web Inspector: Styles Redesign: turn on CSS spreadsheet editor by default
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Nikita Vasilyev
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-10-31 19:08 PDT by Nikita Vasilyev
Modified: 2017-11-02 13:46 PDT (History)
5 users (show)

See Also:


Attachments
Patch (12.69 KB, patch)
2017-11-01 18:38 PDT, Nikita Vasilyev
bburg: review-
Details | Formatted Diff | Diff
[Animated GIF] With patch applied (283.53 KB, image/gif)
2017-11-01 18:41 PDT, Nikita Vasilyev
no flags Details
Patch (14.41 KB, patch)
2017-11-02 12:23 PDT, Nikita Vasilyev
bburg: review+
Details | Formatted Diff | Diff
Patch (14.46 KB, patch)
2017-11-02 13:13 PDT, Nikita Vasilyev
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nikita Vasilyev 2017-10-31 19:08:38 PDT
- Make the redesigned styles sidebar (spreadsheet editor) on by default.
- Replace Experimental Spreadsheet Style Editor checkbox with something like "Styles Editing: [ ] Use legacy styles sidebar".
- The legacy styles sidebar has setting:

Styles Editing: [x] Show inline warnings
                [x] Automatically insert newline
                [x] Select text on first click

They are not applicable to the redesigned styles sidebar. I think we should remove them entirely.
Most Web Inspector users don't even know about the settings. I expect very few people to use the legacy styles sidebar AND
have custom settings for it.

<rdar://problem/33524171>
Comment 1 Radar WebKit Bug Importer 2017-10-31 19:09:03 PDT
<rdar://problem/35285990>
Comment 2 Nikita Vasilyev 2017-11-01 18:38:53 PDT
Created attachment 325660 [details]
Patch
Comment 3 Nikita Vasilyev 2017-11-01 18:41:08 PDT
Created attachment 325661 [details]
[Animated GIF] With patch applied
Comment 4 Haroen Viaene 2017-11-02 06:56:52 PDT
Some feedback here, if I enable the spreadsheet editor now, it doesn't create a `property: value` if I type that in the property part, it requires me to tab into the value part, would be nice to automatically switch to the value input when a : is typed. 

Thanks!
Comment 5 Brian Burg 2017-11-02 11:34:42 PDT
(In reply to Haroen Viaene from comment #4)
> Some feedback here, if I enable the spreadsheet editor now, it doesn't
> create a `property: value` if I type that in the property part, it requires
> me to tab into the value part, would be nice to automatically switch to the
> value input when a : is typed. 
> 
> Thanks!

Already filed: https://bugs.webkit.org/show_bug.cgi?id=178795

In general, you should just file a new bug if your feedback are not related to the patch in a bug. Thanks!
Comment 6 Brian Burg 2017-11-02 11:38:10 PDT
Comment on attachment 325660 [details]
Patch

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

> Source/WebInspectorUI/ChangeLog:9
> +        Add a "Legacy Style Editor" checkbox and remove the experimental "Spreadsheet Style Editor" checkbox.

I don't think this should be in the General section. Let's just move it to Experimental or Debug until we completely remove it. (Is there any reason to not remove it right now?)
Comment 7 Brian Burg 2017-11-02 11:42:46 PDT
Comment on attachment 325660 [details]
Patch

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

The changes to remove old settings look fine. I want this in Experimental though, so that we get more feedback and its clear we don't plan to maintain it any more.

> Source/WebInspectorUI/UserInterface/Views/SettingsTabContentView.js:209
> +        reloadInspectorButton.addEventListener("click", () => { window.location.reload(); });

This should show up at the bottom of the settings pane. It looks weird putting it inline. I think Experimental section has a reusable implementation already, so you should be able to just remove this.
Comment 8 Nikita Vasilyev 2017-11-02 12:22:04 PDT
(In reply to Brian Burg from comment #6)
> Comment on attachment 325660 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=325660&action=review
> 
> > Source/WebInspectorUI/ChangeLog:9
> > +        Add a "Legacy Style Editor" checkbox and remove the experimental "Spreadsheet Style Editor" checkbox.
> 
> I don't think this should be in the General section. Let's just move it to
> Experimental or Debug until we completely remove it. (Is there any reason to
> not remove it right now?)

I can move the setting to the Experimental section. As long as the checkbox is available for everybody, it's fine with me.

I don't want to remove it just yet. I want to remove it once we receive a good amount of positive feedback.

The new styles sidebar still misses some features and lacks polish. If somebody find the new styles sidebar unusable for their workflow I want to allow them to switch back to the old one without running a different version of WebKit.
Comment 9 Nikita Vasilyev 2017-11-02 12:23:06 PDT
Created attachment 325744 [details]
Patch
Comment 10 Brian Burg 2017-11-02 12:54:29 PDT
Comment on attachment 325744 [details]
Patch

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

r=me

> Source/WebInspectorUI/UserInterface/Base/Setting.js:126
> +    legacyStyleEditor: new WI.Setting("legacy-style-editor", false),

Nit: experimental settings need experimental- prefix.
Comment 11 Nikita Vasilyev 2017-11-02 13:13:46 PDT
Created attachment 325760 [details]
Patch
Comment 12 WebKit Commit Bot 2017-11-02 13:46:14 PDT
Comment on attachment 325760 [details]
Patch

Clearing flags on attachment: 325760

Committed r224354: <https://trac.webkit.org/changeset/224354>
Comment 13 WebKit Commit Bot 2017-11-02 13:46:15 PDT
All reviewed patches have been landed.  Closing bug.