[KCMs/Notifications] Tweak layout and strings to have a bit more visual grouping for the logical sections
ClosedPublic

Authored by ngraham on Jan 2 2020, 10:14 PM.

Diff Detail

Repository
R119 Plasma Desktop
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
ngraham created this revision.Jan 2 2020, 10:14 PM
Restricted Application added a project: Plasma. · View Herald TranscriptJan 2 2020, 10:14 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
ngraham requested review of this revision.Jan 2 2020, 10:14 PM

Also while looking at this, I started to think the checkbox for critical notifications maybe doesn't even need to exist. We probably shouldn't make it easy to shoot yourself in the foot by disabling the display of critical notifications. They're critical for a reason, after all!

ngraham retitled this revision from [KCMs/Notifications] Add a bit of grouping for the logical sections to [KCMs/Notifications] Tweak layout and strings to have a bit more visual grouping for the logical sections.Jan 2 2020, 10:18 PM
broulik accepted this revision.Jan 2 2020, 10:19 PM
This revision is now accepted and ready to land.Jan 2 2020, 10:19 PM

by disabling the display of critical notifications.

It doesn't disable them. Merely keeps them behind full screen windows. I think the option is fine as it is. (You know, if we had some kuserfeedback data to prove that nobody disables this option... :p)

ndavis added a subscriber: ndavis.Jan 3 2020, 1:54 AM

Is it intentional that "Hide after" is aligned with the checkboxes above? I'm not convinced this part is an improvement. Same with the indented "Toggle with" although that was there to begin with.

+1 to the rest of these changes.

The indentation here was done to be consistent with the other one, but the reason why they're indented is to make it clear that they're a part of the logical group of controls that's "introduced" by the first left label in the formlayout section. Ideally every section has only a single one of these labels that stands in as a sort of pseudo-header for the whole section. I'm not super happy with the two groups in this layout that have two left labels, but I couldn't figure out a way to get rid of them without removing options.

ndavis added a comment.EditedJan 3 2020, 3:47 AM

Within that context, I see why it makes sense to indent "Toggle with", but I still don't think it makes sense to indent "Hide after"/"Hide popup after". The label above is "Popup position" and "Hide popup after" is not related to the popup position.

Would it make more sense if it was like this?

Popup: (o) Show near notification icon
       ( ) [Choose custom Position...]
       Hide after: [5 seconds ^v]
ndavis added a comment.Jan 3 2020, 6:43 AM

Would it make more sense if it was like this?

Popup: (o) Show near notification icon
       ( ) [Choose custom Position...]
       Hide after: [5 seconds ^v]

Yes, that would make perfect sense.

ngraham updated this revision to Diff 72690.Jan 3 2020, 3:12 PM

Do that

ngraham edited the test plan for this revision. (Show Details)Jan 3 2020, 3:13 PM
ndavis accepted this revision.Jan 5 2020, 6:25 AM
This revision was automatically updated to reflect the committed changes.