backdrop / backdrop-issues

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

[UX] - Clarify visibility conditions vs access conditions in Layout interfaces. #6705

Closed stpaultim closed 1 month ago

stpaultim commented 1 month ago

Description of the bug

I've been working on a couple of issues to help folks understand the relationship between paths and layouts, in specific in relationship to this new feature in core. https://github.com/backdrop/backdrop-issues/issues/1929

Specifically, I opened this issue: https://github.com/backdrop/backdrop-issues/issues/6699

It has been pointed out that my use of "visibility conditions" in the text, might be better expressed as "access conditions" in relationship to paths. Which brings me to this inconsistency in the current UI (or at least what appears to be an inconsistency).

image

Questions:

Is there a good reason for this inconsistency between the use of visibility and access on this specific page? Can we fix this on this page alone or does this have much broader implications?

It is my current thought, that "visibility conditions" does make sense for blocks, but for paths the word "access conditions" is more accurate. I'm worried that this might make the site architect experience more confusing. BUT, it may also help a site architect better understand what is going on here.

As I begin to think about it this way, I think it's helping my understanding.

Expected behavior

Would it be better if this page looks like this (changing link from "Add visibility condition" to "Add access condition")? Does this have ramifications for other parts of the UI?

image

AND if so, should this be change from "Visibility Conditions" to "Visibility or Access Conditions"?

image

OR is this even worth worrying about?

Atten: @jenlampton, @quicksketch, @bugfolder

jenlampton commented 1 month ago

Is there a good reason for this inconsistency between the use of visibility and access on this specific page?

Yes, the bottom Add visibility condition is universal, whereas the form element label Access condition appears only on this page.

Can we fix this on this page alone or does this have much broader implications?

Both? I have a crazy idea....

Would it be better if this page looks like this (changing link from "Add visibility condition" to "Add access condition")? Does this have ramifications for other parts of the UI?

Yes, but...

AND if so, should this be change from "Visibility Conditions" to "Visibility or Access Conditions"?

What about simply "Conditions" / "Add condition"?

If we use the distinct labels (unique to each form ) to define what type of condition it is, then a more generic Add condition underneath would fit all Scenarios. See examples below.

Screenshot 2024-09-05 at 5 51 22 PM Screenshot 2024-09-05 at 5 52 07 PM Screenshot 2024-09-05 at 5 52 55 PM Screenshot 2024-09-05 at 5 54 17 PM
quicksketch commented 1 month ago

We discussed this today in the weekly meeting and everyone seems to think simplifying down to simply "Conditions" for the link and table header is a good solution. We'd still leave "Access conditions" and "Visibility conditions" as labels above the form element (as in Jen's screenshots above).

jenlampton commented 1 month ago

PR filed at https://github.com/backdrop/backdrop/pull/4872. This was a very minor change plus lots and lots of test updates, so 🤞

quicksketch commented 1 month ago

This PR looks great. I confirmed it works as expected and test coverage is extensive on Layout conditions, both on blocks and layout configurations, so I'm confident this doesn't break any functionality.

It will however, break existing test coverage within contrib that adds Layout-related capabilities. I was thinking about https://github.com/backdrop-contrib/layout_wildcard (@bugfolder) and https://github.com/backdrop-contrib/mini_layouts (@docwilmot) in particular. But reading through their test coverage, it doesn't look like they use or test the string "Visibility condition" anywhere. So we might luck-out there.

String changes and test coverage are not considered backwards-compatibility breaking changes, so even if it does break contrib test coverage, it's still an acceptable change.

So all together, looks good to me.

quicksketch commented 1 month ago

Thanks @jenlampton and @stpaultim! Merged https://github.com/backdrop/backdrop/pull/4872 into 1.x for 1.29.0.