backdrop / backdrop-issues

Issue tracker for Backdrop core.
144 stars 40 forks source link

[UX] Permissions tables: Warnings are rendered as regular help/description text. #3252

Open klonos opened 6 years ago

klonos commented 6 years ago

For a seasoned Drupal/Backdrop user, this might be obvious because they have either gotten used to be reading the descriptions, or because over time they have memorised which permissions are potentially dangerous. Also, when the warning is the only thing in the permission description, it makes it more discoverable, but if there is actual description for the permission, then the warning gets appended to the description text, which makes it lose its warning status:

screen shot 2018-08-14 at 12 55 20 pm
klonos commented 6 years ago

Screenshot from the PR sandbox:

screen shot 2018-08-14 at 1 12 33 pm
olafgrabienski commented 6 years ago

I like the idea to emphasize the warnings a bit more when they're combined with help text, but the "warning" message is too strong, in my opinion. It would distract from other information on the page and would make it quite difficult to configure the permissions. And, regarding a warning message I would generally expect that I can do something (change configuration, save a form, ecc.) so that the message goes away.

My proposal: don't change the styling class and/or add the warning messages status but when combined with help text, render the warning text in an own paragraph.

klonos commented 6 years ago

I have filed two separate PRs. One makes the changes in user.css, which makes sense from an organisational point; the second makes the style changes in messages.theme.css, which will make it easier to make changes in a single place in the future. Not sure which is best, but both have the same end result, so pick and choose 😉

klonos commented 6 years ago

Thanks for the feedback @olafgrabienski 👍

I personally think that styling the warnings about security implications as warnings is what we should be doing. I filed this issue because I was going through the permissions page and noticed that people might be missing these messages if they are not reading the descriptions (most people don't).

Having said that, I just filed yet another alternative PR that simply changes the warnings to be rendered in a separate line (and keeps them as <em>s instead of <span>s). I don't think that this solves the UX problem though, because permission warnings still look like regular help text:

screen shot 2018-08-14 at 1 58 51 pm
olafgrabienski commented 6 years ago

I filed this issue because I was going through the permissions page and noticed that people might be missing these messages if they are not reading the descriptions ...

I see, but many people ignore warnings unconsciously even when they're styled as very important if they appear always (irrespective of individual settings).

I don't think that this [warnings just in a separate line] solves the UX problem though, because permission warnings still look like regular help text.

We can't really "solve" the issue but improvements are better than nothing. We could make the Warning: at the beginning bold, that will help a bit, I guess.

klonos commented 6 years ago

Sure @olafgrabienski ...I will wait for some more feedback by others and I will update the PRs accordingly.

Tagging design and pinging @wesruv

quicksketch commented 6 years ago

I also think the full "warning" styling is too aggressive. I like the pushing the warning onto a new line. I think that makes it more visible (if you are looking for it). I tried just using the same styling as the span.admin-missing color, but I thought that was a little much as well:

image

I tried just doing the word "Warning" in red, as well as in bold:

image

IMO, the last one, with the word "Warning" bold and on its own line seems like the right balance of getting attention but not distracting from the rest of the page.

jenlampton commented 6 years ago

IMO, the last one, with the word "Warning" bold and on its own line seems like the right balance of getting attention but not distracting from the rest of the page.

I agree, I really like the last option, The word "Warning" in red, but with normal text: image

olafgrabienski commented 6 years ago

I'd prefer the very last one: "Warning" not red but bold.

docwilmot commented 6 years ago

Prefer warning in bold, not red. Red makes it look like something is wrong there with that field element/checkbox.

jenlampton commented 6 years ago

That's a great point @docwilmot. I like having color, but maybe Red isn't the best option here. Yellow would be "better" except that yellow text will be very hard to read :/

Maybe we can use the bold text, but with a yellow ! icon inline? Or maybe even a black ! icon would be sufficient without the use of color?

docwilmot commented 6 years ago

Personally this seems like a solution without a problem. Has anyone ever complained that this has caused them any grief?

klonos commented 3 years ago

I plan to revive this. When mixing a warning about deprecated permissions (#5030) with the help/description text for the permission, the warning loses prominence IMO. Compare this:

image

To this (warning rendered before the description, with the word "Warning" in red):

image

wesruv commented 3 years ago

FWIW, I'm with @docwilmot, I vote bold "Warning:", maybe darken the word warning, but no red.

I don't think I'd want any other icon because there's a lot going on on this page already, and I feel like bolding Warning will make it stand out enough.

klonos commented 3 years ago

I'm fine with bolded as well 👍🏼 ...but also move the warnings before the help text, and in their own line:

image

If we do this, I feel that a single warning message (proper backdrop_set_message()) should be added to the to of the page while there are deprecated permissions still assigned to roles (#5030 / #4487).

klonos commented 3 years ago

I'm fine with bolded as well...

...actually I changed my mind. If we are to show a warning in the status page, and have people come to the Permissions page to fix their permissions, it's really hard to spot the ones that need fixing when the word "warning" is just bolded gray 👎🏼

wesruv commented 3 years ago

Ideally you could solve that issue with an anchor link to those perms, from the warning.

jenlampton commented 3 years ago

I'm with @wesruv. Red won't help for people who have trouble with colors. It would be best if we could solve it for everyone :)

On Mon, Apr 5, 2021, 8:48 AM wesruv @.***> wrote:

Ideally you could solve that issue with an anchor link to those perms, from the warning.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/backdrop/backdrop-issues/issues/3252#issuecomment-813464871, or unsubscribe https://github.com/notifications/unsubscribe-auth/AADBERYYLZRKJKQ7PCFZFXTTHHLWHANCNFSM4FPQSL2A .

klonos commented 3 years ago

Yup, anchor links in the message would work I guess 👍🏼

...but if there are multiple deprecated permissions all over the place, the user would need to either scroll up/down multiple times (to click the links on the message, at the top of the page), or resort to fixing one permission at a time (click anchor link -> untick checkboxes -> submit the form -> repeat as many times as there are deprecated permissions).

Other solutions would be:

ghost commented 2 years ago

I agree that permission warnings should be more prominent, and I like the simple approach of making the warnings show on a new line and with a bold 'Warning' prefix. I think the majority also prefer this.

I therefore recommend going forward on this issue with that approach, and opening separate issues for other suggestions (e.g. deprecated permission warning, etc.).

@klonos Can you please clarify which (if any) of your PRs address the proposed solution here, and can you create separate issues for your other suggestions?

klonos commented 5 months ago

Apologies for dropping the ball on this 😞 ...I've closed all other PRs (outdated and had multiple conflicts), and filed a fresh one here: https://github.com/backdrop/backdrop/pull/4762

Things have changed since this issue was originally opened, and we don't have the problem of the warning being in the same line as the description any more (see #5536). So the PR simply makes the word "Warning" bold, since that was the consensus by most people here. Side-by-side before/after screenshot: image

stpaultim commented 5 months ago

@klonos - I was planing on being bold and doing both testing and code review for this PR and marking it RTBC. But, I found a small surprise that I didn't notice any discussion about.

image

The PR adds a second warning to the warning already put on Filters. This feels weird to me.

This is how these permissions looked before:

image

Having the word "warning" starting two lines in a row feels redundant and distracting. I think a single warning is sufficient. My recommendation would be to do one of the following.

Stick with the single warning that existed before, the one that includes "depending on how the text format is configured."

OR

Do what you are currently doing, but don't use the word "warning" a second time (and possibly shorten text a bit). See:

image

"The security implications will depend on how this text format is configured."

I'm not convinced the extra line of text is necessary. But, if we are going to have it, then I think my suggestion is a bit shorter and gets the point across without redundancies.

klonos commented 5 months ago

The PR adds a second warning to the warning already put on Filters. This feels weird to me.

It is weird. I thought that I had already taken care of that. Looking again 👀

klonos commented 5 months ago

...oh, I see what's wrong. Working on it 🔧 🔨

klonos commented 5 months ago

The security implications will depend on how this text format is configured.

Yup, I like that suggestion for the description of the warning 👍🏼 ...it shortens the text, while at the same time retaining the same meaning as before, and avoiding redundant use of the same words used in the wording of the actual warning.

PR updated.

stpaultim commented 5 months ago

Works for me, but since this latest change hasn't gotten much input, I'd like to hear from at least one other person that they like this change:

image