[UX][D8] Views "Add fields" modal: Split item title/category/description into separate columns, and streamline JS used

[laryn]

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_checkbox it 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:

1. Go to a View and click on "Add" in the "Fields" or "Filter criteria" section.
2. Scroll the list.

Screenshot (before)

Screenshot (after)

[avpaderno]

I prefer the category: field order to field: category because it is easier to notice that Content revision: Title is not Content: Title.

[avpaderno]

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.

[laryn]

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:

[avpaderno]

That is much better! There is no even the need to "parse" the text to look at the field name.

[laryn]

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?

[avpaderno]

Using the default Backdrop theme, this is what I see.

There are lines around the check-boxes which should not be there.

[olafgrabienski]

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:

[laryn]

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

[olafgrabienski]

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.

[avpaderno]

It looks good to me too.

[laryn]

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]

@kiamlaluno @olafgrabienski Are you willing to review again?

[olafgrabienski]

@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:

[laryn]

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?

[olafgrabienski]

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:

[laryn]

@olafgrabienski Is that on your local with manual patching, or in the Tugboat sandbox? Here's what I'm seeing in the sandbox:

[olafgrabienski]

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.

[laryn]

@olafgrabienski Here's what I see in Chrome on Mac in the sandbox generated by the PR:

[laryn]

@klonos I had considered backdrop_html_id() but discarded the idea because it would make sure the ID was unique so it wouldn't match the checkbox ID -- but hadn't considered using backdrop_html_class() as a stand-in. It seems to work. (PR updated)

[klonos]

... I had considered backdrop_html_id() but discarded the idea because it would make sure the ID was unique so it wouldn't match the checkbox ID

Hmm 🤔 ...are you sure about this @laryn? From what I can see, the checkboxes have the same data-tableselect-id attribute (👍🏼) but they have different id attributes, so it should be fine. The attribute that we use in the labels is for, and it doesn't hurt if these aren't unique - they just need to match the id attribute of the respective checkbox. If you have already tried and it doesn't work, then that's fine. I just left a comment in the PR to add something in the inline comment to explain things though.

[klonos]

...oh, I see, you must be referring to this:

Prepares a string for use as a valid HTML ID and guarantees uniqueness. ... This function ensures that each passed HTML ID value only exists once on the page. By tracking the already returned ids, this function enables forms, blocks, and other content to be output multiple times on the same page, without breaking (X)HTML validation. ...

But if we store that in a variable and we then use the same value for dirrect attributes, then that should be fine. ...or should it? ...Imma go test things 😅

[laryn]

@klonos I've added the additional text you requested in the comment.

[klonos]

Thanks @laryn 🙏🏼 ...I think that this is good to go 👍🏼

I have thoughts, but these are more broad and touch on inconsistencies with the FAPI etc. - so not here - follow-ups.

[quicksketch]

I resolved a merged conflict in the PR (we stopped using $.trim() thanks to #6149). This is sort of a big set of changes but seems as though it's had extensive testing from @klonos, @laryn, @kiamlaluno, @stpaultim, and @olafgrabienski.

[quicksketch]

I retested the PR and everything seems to work as described.

I've merged backdrop/backdrop#4628 into 1.x and 1.28.x. Thank you everyone!

bf8e45e by @laryn, @kiamlaluno, @klonos, @olafgrabienski, @stpaultim, and @docwilmot.