backdrop-issues
backdrop-issues copied to clipboard
backdrop
Clean up and harden Views "Add fields" modal
Description of the bug
While working on Gin I noticed that the Views modal for selecting fields or filters could use some clean up. Here are some of the things I'd like to improve:
- Each field is prefaced by the category, which makes it hard to read field names/labels at a glance. In Drupal 10 the category is in a completely separate column after the field label, ~but perhaps it would be enough to just put it after the field label? I'm curious if others agree that this is harder to read with the category inserted at the front.~ and the latest PR replicates that.
- The script that shows which fields have been selected could be made a little less fragile. If a theme is overriding
theme_checkboxit is very easy to break that (as Gin does) and it can be hardened with a minor tweak. Right now it looks for the label exactly following the checkbox, but (for example) Gin adds a span directly after the checkbox, so the "Selected: " interface is broken. ~Changing that to look for a sibling label instead of whatever happens to be directly following seems a little stronger.~ Code from Drupal 10 has been adapted to match the corresponding structural changes and is more robust.
Steps To Reproduce
To reproduce the behavior:
- Go to a View and click on "Add" in the "Fields" or "Filter criteria" section.
- Scroll the list.
Screenshot (before)
Screenshot (after)
I prefer the category: field order to field: category because it is easier to notice that Content revision: Title is not Content: Title.
Leaving out that, in English and other left-to-right languages, the order would be <generic category>: <specific category> or <phrase>: <explanation of the previous phrase>, <field>: <category> could help if at least the fields were ordered by <field> and <field>: <category> were used in the view page too.
With the PR, the fields list is not easier to read (for a left-to-right language) than the list as it shown now.
If the list showed Title: Content and Title: Content revision as consecutive items, at least I could notice there are more Title fields without reading the full list.
I wonder if we can break these out separately instead of mashing them together. For reference, here's a screenshot of how D10 does it, using a table and individual <td>'s:
That is much better! There is no even the need to "parse" the text to look at the field name.
This is a screenshot from my local (with Gin for Backdrop as the theme) with the latest changes to the PR:
I reworked it to use a table and td separation, and largely copied a section of code from Drupal 10 to rework the search functionality to work in this new structure. (EDIT: for code review, here's a relevant section of code from Drupal 10 where much of this came from).
@kiamlaluno (and @olafgrabienski -- I see that thumbs up), are you able to test this and post your feedback here?
Using the default Backdrop theme, this is what I see.
There are lines around the check-boxes which should not be there.
I've applied the PR locally and I really like the general look and feel. See the screenshots below for a comparison in Seven. As mentioned by @kiamlaluno, I also see the lines around the check boxes in the Seven theme, btw.
There seems to be a regression with regards to the Search box. Without the PR, you could filter the Category by typing parts of the Category name into the search box. With the PR, this doesn't work anymore. If I didn't miss anything, the search box looks only for Title and Description now. (To test, put "Global" in the search box.)
Screenshots ("Add field" dialog in Seven)
(a) Without PR 4628:
(b) With the PR:
Thanks @kiamlaluno and @olafgrabienski -- I've made a revision to the PR. It does change the display slightly for Seven, with fewer high contrast border lines, making it more in line with the way Seven displays the main admin content listing.
I've also added the ability to keyword filter on the group name.
Seven
Gin
Thanks for the update, @laryn. The revised PR works for me:
- Checkboxes look good in Seven (and Gin).
- Search box works with Categories.
I've also had a look at the line with "Selected" fields. Looks good both in Seven and in Gin.
Score one for automated testing which has revealed a breakage if you actually try to go further along from here... I'll have to come back to this one!
@laryn Sure! I've tested in the sandbox, and the PR looks good at first sight (see 1st screenshot below). But I've noticed an issue, not sure if new or if it existed before: If you don't filter the fields, the "Selected" line is somehow hidden (see 2nd screenshot). This doesn't happen with a fresh Backdrop demo.
Screenshot 1:
Sceenshot 2:
Thanks for testing again @olafgrabienski!
This is a fresh site with nothing selected:
With the current PR, with nothing selected:
In both cases, the "Selected" text does not show up when nothing is selected. Are you seeing something different?
In both cases, the "Selected" text does not show up when nothing is selected. Are you seeing something different?
No, in this case I see the same. I was referring to another situation but didn't make that clear (sorry): In my second screenshot I had selected some fields (the same as in the first screenshot). The selected checkboxes are not visible, though, because their rows are further down in the not visible part of the dialog.
I'm adding a new screenshot with some visible selected checkboxes to make the issue more clear. Note also the highlighted region in the screenshot where you see a small fraction of "Selected" line:
@olafgrabienski Is that on your local with manual patching, or in the Tugboat sandbox? Here's what I'm seeing in the sandbox:
Is that on your local with manual patching, or in the Tugboat sandbox?
Also sandbox ... Maybe a browser question? I was testing with Chrome on Mac.
@olafgrabienski Here's what I see in Chrome on Mac in the sandbox generated by the PR:
Hm, testing with Firefox for me it looks similar to Chrome, but it gets better at a certain point when I reduce the height of the browser window.
I checked the PR preview on Firefox, on a computer running on SparkyLinux (based on the testing branch of Debian). On a 1920x1080 screen, I see the dialog box as expected.
- Without selected fields:
- With selected fields
My screen is quite small (MacBook Air M2, standard resolution 1470x956). In the browser's dev tools I see the actual height within the browser is 832px. When I reduce the actual height to less than 750px, the dialog becomes smaller, so it shows less rows and lets enough space for the "Selected" line. Screenshot comparison in Chrome:
Actual size in browser window, 1470 x 832px:
Actual size in browser window, 1470 x 749px:
If you don't filter the fields, the "Selected" line is somehow hidden. This doesn't happen with a fresh Backdrop demo.
I have to correct myself. Now I see the same issue in the fresh Backdrop demo site without the PR, so not introduced by this PR. When I had compared the PR with the demo site before, I didn't pay attention to the browser window height. Sorry that I didn't compare more thoroughly.
@olafgrabienski I see the same thing -- care to split that off into a separate issue for separate discussion then?
split that off into a separate issue for separate discussion then?
Agreed! (I put it on my list.)
I reduced the viewport on Firefox, and I noticed the same issue, when I use a 1470x750 screen. With a 1470x956 viewport, nothing changes respect a 1920x1080 viewport.
Thanks @laryn 🙏🏼 ...I've done a quick review of the code and added a couple of suggestions in the PR thread.
I've filed a separate issue re the visibility of the "Selected" line: #6395
@klonos FYI, I moved it back to Needs Code Review since I wasn't sure if you were done with your review.
@laryn I recall not being able to finalize the review because of the extend of the changes in views-admin.js. Do you think that we could split the indentation fix for that file into a separate issue/PR in order to make this easier to review?