backdrop-issues
backdrop-issues copied to clipboard
backdrop
[UX] Layout reorder page: add "Cancel" link, and indicate things like machine names and enabled/disabled status
This is what that page currently looks like:
...and if no layouts have been specified:
I would like us to:
- Change the page title to "Reorder layouts" (possibly also the operation link that leads here)
- Add a proper breadcrumb (currently only shows "Home")
- Indicate which path this is about
- Indicate machine names (using the new
theme_label_machine_name()function) - Indicate disabled status for any disabled layouts in the table
- Add a "Cancel" link
The "Reorder" option is not available OOTB in the layouts listing page. If you want to test/review this, an easy way to get the "Reorder" button is to clone a couple of existing layouts. That makes it so that there are multiple layouts for the same path, and reveals that button.
...whinging (separate issues):
- I don't like how
layout_reorder_form()is detecting layouts from the URL via$_GET['layouts'], which results in ugly URLs (admin/structure/layouts/reorder?layouts[0]=layout_one&layouts[1]=layout_two&layouts[2]=layout_three...which browsers decode toadmin/structure/layouts/reorder?layouts%5B0%5D=layout_one&layouts%5B1%5D=layout_two&layouts%5B2%5D=layout_three) and is more prone to errors. Although nowadays the length of URLs is not an issue, I would prefer it if we passed the layouts as parameters to that function instead, or even better pass the path of the layout menu item in question (and then detect all layouts configured for that path). - I would prefer it if we actually merged the reorder form with the main layouts listing, so that you could reorder all reorder-able layout sets from the same place.
I co-whinge on both points.
Particularly on the second point. I'd never had reason to try to reorder layouts before, and when testing this PR, it took me a while to find the "REORDER" button hiding in the right column of similarly-looking buttons and dropdowns (which have their names rendered in ALL CAPS to reduce readability). I would have expected that if layouts were reorderable, they'd be reorderable from within the main layout listing, as you describe.
Do you want to proceed with the current PR as an incremental improvement?
Do you want to proceed with the current PR as an incremental improvement?
Yes, I do. It might prove that the 2 points I'm complaining about a) don't ever get any traction, and b) prove much harder to solve. I do plan to raise separate follow-ups for them once this is in though.
Let's get it in, then.
Tested: created several layouts for path home. Checked:
- Page title is "Reorder layouts at path home"
- Breadcrumb is Home > Administration > Structure > Layouts
- Machine names are visible under layout names
- Disabled layout shows up grayed out
- There is a Cancel link.
As it should be. However:
- Created a second layout at path
admin/dashboard. - Went to Reorder the layouts for that path.
- Leaving that window open, in another window, deleted the second layout.
- Refreshed the still-open Reorder page (expecting that it would now only show a single row).
- Instead, it shows two rows, one of them blank, like this:
This is presumably because of the weird way that the layout page is specified in the URL, which is:
admin/structure/layouts/reorder?layouts%5B0%5D=dashboard&layouts%5B1%5D=dashboard_1
which is your first whinge above.
In the interest of making progress, I'd be inclined to say "yes, let's proceed with this one," but then add the issue of changing the path for layout reordering to something like admin/structure/layouts/reorder?path=path/to/be/reordered, or something like that.
I can't really "whinge" about this, since I just looked up the word a few minutes ago, it doesn't seem fair to engage in it so quickly. I might "whine" about this, except that I'm not sure I even knew that this page even existed. It's never occurred to me that I might need to reorganize my layouts, now I'm worried about what I might have been missing out on.
I like all the changes that this PR makes. Maybe, I will take advantage of them in the future.
I verified that the problem mentioned by @bugfolder in his last comment, pre-dates this PR. Given that, I would assume that it's ok to proceed and deal with that on it's own.
I'm going to mark this "Works for me" and see what happens. ;-)
I've reviewed the code; looks fine.
I've created a follow-on issue to move reordering into the main Layouts page: https://github.com/backdrop/backdrop-issues/issues/5411. That would address the issue I raised above (also the author's Whinge Number 1), which seems beyond the scope of this issue so doesn't need to be addressed here.
So, WFM, Code Reviewed, and therefore RTBC.
In the interest of making progress, I'd be inclined to say "yes, let's proceed with this one," but then add the issue of changing the path for layout reordering to something like
admin/structure/layouts/reorder?path=path/to/be/reordered, or something like that.
Or just go ahead and fix things now 😅 ...PR updated to do exactly that 😉 (and also rebased, since the PR was originally created back in October).
Can you imagine the "ugliness" of the URL if you had to reorder a dozen of layouts or more? ...not to mention the possibility of hitting the max URL length (although far-fetched).
Anyway, here's what it looks like now:
/admin/structure/layouts/reorder(no layout path provided) or/admin/structure/layouts/reorder?layouts%5B0%5D=dashboard_clone&layouts%5B1%5D=dashboard(layouts instead of path passed in the URL query):
/admin/structure/layouts/reorder?path=abc123(arbitrary path passed):
/admin/structure/layouts/reorder?path=homebut only one layout exists on that path:
/admin/structure/layouts/reorder?path=admin/dashboardwith multiple paths available on that path:
Please review and let me know if the wording in the messages needs to be tweaked further.
Some suggestions on the above:
/admin/structure/layouts/reorder(no layout path provided) or/admin/structure/layouts/reorder?gibberish:- This seems like it warrants an error message, not a warning message
- How about "No layout path was provided." (Complete sentence error messages are nice.)
/admin/structure/layouts/reorder?path=abc123(arbitrary path passed):- Also seems like it warrants an error message, not warning message
- How about "There are no layouts with path abc123." (Shorter, doesn't matter whether layouts are reorderable; the main problem is that no layout exists for this path.)
/admin/structure/layouts/reorder?path=homebut only one layout exists on that path:- This seems like it warrants a warning (or error), not a status message.
- How about "There is nothing to reorder because there is only one layout with path home."
/admin/structure/layouts/reorder?path=admin/dashboardwith multiple paths available on that path:- How about "The selected layout for this path will be the first layout in the list below whose conditions are satisfied. Reorder the layouts to affect the order of condition evaluation."
Thanks @bugfolder ...very valid observations/suggestions. I'll revise the PR soon as possible.
/admin/structure/layouts/reorder(no layout path provided) or/admin/structure/layouts/reorder?gibberish:
- This seems like it warrants an error message, not a warning message
- How about "No layout path was provided." (Complete sentence error messages are nice.)
I ended up doing something that would be more useful in this situation, which is to provide a list of layouts to choose from:
So it still feels OK to have a warning instead of an error. Right?
/admin/structure/layouts/reorder?path=abc123(arbitrary path passed):
- Also seems like it warrants an error message, not warning message
- How about "There are no layouts with path abc123." (Shorter, doesn't matter whether layouts are reorderable; the main problem is that no layout exists for this path.)
I like the text suggestion 👍🏼 ...but similarly to when no layout path has been provided, we can be helpful and provide the same list of layout paths to select from (although slightly different page title and warning message):
/admin/structure/layouts/reorder?path=homebut only one layout exists on that path:
- This seems like it warrants a warning (or error), not a status message.
- How about "There is nothing to reorder because there is only one layout with path home."
Yup 👍🏼
/admin/structure/layouts/reorder?path=admin/dashboardwith multiple paths available on that path:
- How about "The selected layout for this path will be the first layout in the list below whose conditions are satisfied. Reorder the layouts to affect the order of condition evaluation."
I find to affect the order of condition evaluation rather vague/cryptic. So I went with this instead:
When there are multiple layouts available for the same path, the first one that satisfies its visibility conditions is selected. Reorder the layouts on this path to affect which layout will be preferred.
Tested, works great:
- Changes the page title to "Reorder layouts at path admin/dashboard" (for example)
- Adds a proper breadcrumb (shows the full breadcrumb trail)
- Indicates which path this is about (admin/dashboard)
- Indicates machine names
- Indicates disabled status for any disabled layouts in the table
- Add a "Cancel" link (and it works).
I have one teeny request for wording change. From
When there are multiple layouts available for the same path, the first one that satisfies its visibility conditions is selected. Reorder the layouts on this path to affect which layout will be preferred.
to
When there are multiple layouts available for the same path, the first one that satisfies its visibility conditions is selected. Reorder the layouts on this path to affect which layout will be chosen.
Emphasis above just to point out the word change. I'm fine with either, if you prefer (choose?) the original.
Just waiting on a response to the "preferred" vs "chosen" question (2 comments up) before marking this RTBC.
I thought this would a quick win by changing "preferred" to "chosen", but there are conflicts of this PR with core. Particularly with the $path_groups being moved to a new function in layout.admin.inc. That has changed in core.
