Add: style settings

[eight04]

Fixes #827.

There are some features linked to #827 are closed without implementation. Do you want to create a new meta thread to track them or I will just re-open the old ones?

User inclusion doesn't work either.

[eight04]

[tophf]

Great, but I dislike the placement. There's too much info already so I think this UI should be shown in a modal after pressing a button in the left panel. It will probably allow showing it from the manager too.

The current Options block should be moved to another modal dialog too to reduce the density of info in the panel.

I would also remove "Back to manage". If the user opened the editor from the manager then they can use the back button. Otherwise they can open the manager in a new tab.

The "style should be applied" selector is too wide, it should be either auto-sized like it's done in the manager or it shouldn't be a selector, but instead a set of radio inputs. I like the latter more because the options are visible.

The button to remove a rule should be plain like the x in the manager or - in the applies-to widget, or it should be a standard button with a readable label like Delete.

The block of included/excluded sites should be somehow grouped, maybe using <ol> and <li>.

@narcolepticinsomniac, hopefully you can come up with less technical labels for the inclusion/exclusion. My feeling is to remove rule because I don't see anything reminding a rule. Maybe just Custom included sites, Custom excluded sites?

[narcolepticinsomniac]

I agree it'd be better in a modal. Options probably would be too, but that's not really relevant here. Not sure I see the point of removing the manager button either, as it's simply a navigational convenience.

Tophf's suggested tweaks for the actual per-style settings UI are all on point.

As far as the include/exclude buttons, I'd suggest getting rid of them altogether, and leaving an initial input in its place.

These sections could, and probably should, operate the same as regular applies-to widgets.

[eight04]

These sections could, and probably should, operate the same as regular applies-to widgets.

Note that this PR adds zero features. I just added the setting page to show them. Those exclusions were implemented in #724 which are actually glob patterns.

Modal, button, etc.

IIRC someone had requested a redesign for the editor. Let's handle them in another PR.

[narcolepticinsomniac]

this PR adds zero features

Ok, but... it's going to have to eventually, correct? AFAIK, practically all these per-style settings don't work currently, so I assumed you're getting the UI out of the way first, in order to focus on the rest.

If the goal here is to settle on a UI, we need to know the functionality. In terms of user includes/excludes, we have the overly simplified idiot-proof version in the popup, because it's toggle-based, and also because space is limited.

If space is not an issue, and we're giving the user proper inputs to craft their own include/exclude rules, then they should work the same way applies-to rules have always worked, and the UI should most likely be the same as well.

IIRC someone had requested a redesign for the editor. Let's handle them in another PR.

Why? This seems like the perfect PR to decide where they should go.

[eight04]

it's going to have to eventually, correct?

Yes. I'll start working with priority and inclusion after merging this.

practically all these per-style settings don't work currently

Update URL, dark/light mode, and exclusions are already implemented. These features already work in master.

IIRC someone had requested a redesign for the editor. Let's handle them in another PR.

Why? This seems like the perfect PR to decide where they should go.

So we can development other features simultaneously. For example, the priority feature takes 2 hours and the inclusion feature takes 20 days. If we make them in a single PR, we will have to wait 20 days for the priority feature.

And I predict that redesigning a UI may take even longer like #636.

[narcolepticinsomniac]

redesigning a UI may take even longer

Alright then. Any bickering about the UI can be put off until there's something worth redesigning around anyway.

Includes/excludes need to be a more advanced version of these features to warrant their inclusion, IMO. Otherwise, the dumbed-down toggle version is more convenient in the popup.

[tophf]

"Custom included sites" and "Custom excluded sites" should be somehow grouped. The titles should probably be in bold and maybe inside <details>.

No need for fully spelled buttons, "Add new inclusion", "Add new exclusion", "Delete". IMO they add an unnecessary cognitive load. Just + and - as in our applies-to widget.

Actually, why not just use a textarea where each line is a site rule, like Violentmonkey/Tampermonkey do? No need for the buttons or other complications. It will also allow easier copypasting of the sites between styles.

Priority input's width should be limited, it should use min, max, step, and probably our enforceInputRange.

Includes/excludes need to be a more advanced version of these features to warrant their inclusion

For a start it's fine because it a) lists all the sites conveniently and b) allows adding the sites in the same place.

[eight04]

Actually, why not just use a textarea where each line is a site rule, like Violentmonkey/Tampermonkey do?

Textarea should be a better, especially for wrapping long lines. The only issue is that we have to parse textarea into a list when saving and format a list into text when reading. Users will lose some formatting e.g. empty lines. Though I think the pro overcomes the con in this situation?

[tophf]

We can preserve empty lines, it doesn't cost us anything valuable.

[tophf]

I mean we can store the text as is internally, and parse it when reading the db.

[eight04]

stylus/background/style-manager.js

Lines 373 to 377 in a59aab7

	const list = style[type] || (style[type] = []);
	if (list.includes(rule)) {
	throw new Error('The rule already exists');
	}
	style[type] = list.concat([rule]);

What about the backend? We will probably need something like:

const text = style[prop];
if (new RegExp(`^\s*${escapeRe(rule)}\s*$`).test(text)) {
  throw new Error(...);
}
text += `\n${rule}`;
style[prop] = text;
compileExclusion(style);
saveStyle(...);

[tophf]

By "we can store" I mean in the actual db. When the styles are initialized, the text is converted into a list. The UI here shows the original text. When it's saved, the text will be transformed into a list again, automatically, in styleMan.

[eight04]

• Currently include/exclude just converts list <-> text. We can store the actual text data in another PR.
• Removed priority. Seems that we won't use it in the future.
• Removed codeIsUpdated. While fixing live preview bug I found that it is only used when reason === 'toggled', so I removed it in favor of reason.

[eight04]

[tophf]

Compact mode is broken for the sectioned editor:

I think the simplest fix is to add a button as I suggested initially and show the UI in a modal so it's not bound to the editor's layout.

[eight04]

Are you working on it or let me fix the CSS later?

[tophf]

You try it :-)

[tophf]

event-emitter.js should be moved inside base.js.

The rest of these added script and link should be removed from html, and they should be loaded on demand using require after the user clicks the button to show the settings in a modal.

[tophf]

Until we have a build system it's important to keep the amount of files loaded synchronously to a bare minimum that's necessary to show the editor.

[eight04]

You try it :-)

Did you tell me to try to fix the CSS or try to revert the tab panel and switch to modal? If it is the latter, next time please explain more details.

[tophf]

My comment says everything:

Compact mode is broken for the sectioned editor

I think the simplest fix is to add a button as I suggested initially and show the UI in a modal so it's not bound to the editor's layout.

There are several problems that need to be fixed in this PR: the bug, the modal, the extra files. The bug will disappear if we revert the changes to UI and re-implement it as a modal.

[tophf]

Ah, I think I understand: you couldn't reproduce the bug and you've assumed that I'm referring to the way the UI is scrolled in compact mode and so you started to rework it. I'm not sure I see how this is my fault though and we could avoid it altogether if you implemented the UI as a modal like I immediately suggested.

[eight04]

Don't mind. Let's double check the detail in case the message is too short. I didn't explain what "fix the CSS" is either.