Chore/admin semantic table rows #5950

MadelineCollier · 2024-11-25T14:20:37Z

Summary

This PR is for issue: [Admin] Table component rows do not contain links

This addresses the major concerns of that issue, but does not remove the click handler as it's still required in one case. That is the case of the <tr> clicks. I tried inserting an <a> to wrap the whole row, but that didn't work. The browser refused to render it as it seemingly violates the HTML standard for tables. You can have a link, a td, but not both on the same level within the tr. You can only have both if the link is nested inside the td. We might want to look into inserting it inside of each of the <tr>'s <td>s, but this seems quite messy since some <td>s currently have their own behaviour which would be overruled by that link (row checkbox inputs for one example).

So to recap, this PR addresses our over reliance on the rowClicked stimulus handler for all cases except for the <tr> itself. Now instead of rendering text or divs, we render semantic UI elements (links) with proper hrefs. Hopefully this makes sense @tvdeyen let me know what you think.

Before:

before.mov

After:

after.mov

Checklist

Check out our PR guidelines for more details.

The following are mandatory for all PRs:

I have written a thorough PR description.
I have kept my commits small and atomic.
I have used clear, explanatory commit messages.

The following are not always needed:

📖 I have updated the README to account for my changes.
📑 I have documented new code with YARD.
🛣️ I have opened a PR to update the guides.
✅ I have added automated tests to cover my changes.
📸 I have attached screenshots to demo visual changes.

kennyadsl

Nice! What about using link_to instead?

tvdeyen

Alberto is right, We should use the link_to helper wherever possible.

Regarding the tr click event: I consider this a harmful and bad UX pattern and we should remove it completely.

tvdeyen · 2024-11-25T14:55:42Z

admin/app/components/solidus_admin/ui/table/component.rb

    cell = column.data
    cell = cell.call(data) if cell.respond_to?(:call)
-    cell = data.public_send(cell) if cell.is_a?(Symbol)
+    cell = content_tag :a, data.public_send(cell), href: url if cell.is_a?(Symbol)


We should use the link_to helper here if possible and only if url is present, no?
And why do we still need the cell.is_a?(Symbol) call in case of an url present?

Suggested change

cell = content_tag :a, data.public_send(cell), href: url if cell.is_a?(Symbol)

cell = link_to(data.public_send(cell), url) if cell.is_a?(Symbol)

Sorry for my ignorance, but I am new to this part of Solidus and I still need some time to wrap my head around.

I am also learning as I go as I wasn't the original author of this section, but I think if I understand it correctly the cell that is passed can be one of many types, and the Symbol check is needed in order to know whether we should be calling data.public_send(cell). This method call results in the string which is now newly wrapped in a link, but we still need that string as that is the data we want to show to the user (eg descriptions, names, etc).

admin/app/components/solidus_admin/ui/table/component.html.erb

tvdeyen · 2024-11-25T15:02:46Z

admin/app/components/solidus_admin/users/index/component.rb

@@ -83,15 +83,21 @@ def columns
      },
      {
        header: :order_count,
-        data: ->(user) { user.order_count },
+        data: ->(user) do
+          content_tag :a, user.order_count, href: row_url(user)


link_to seems to be the helper we want here, no?

tvdeyen · 2024-11-25T15:02:52Z

admin/app/components/solidus_admin/users/index/component.rb

      },
      {
        header: :lifetime_value,
-        data: -> { _1.display_lifetime_value.to_html },
+        data: ->(user) do
+          content_tag :a, user.display_lifetime_value.to_html, href: row_url(user)


tvdeyen · 2024-11-25T15:02:57Z

admin/app/components/solidus_admin/users/index/component.rb

      },
      {
        header: :last_active,
-        data: ->(user) { last_login(user) },
+        data: ->(user) do
+          content_tag :a, last_login(user), href: row_url(user)


MadelineCollier · 2024-11-25T15:50:04Z

For sure, I can switch out content_tag for link_to! @tvdeyen what do suggest with regards to my comments in the PR description regarding the issues with removing the click handler for the <tr> elements. We definitely still need these table rows to be clickable as that is essentially the backbone of the new admin flow. Table rows either redirect to individual edit pages or open modals. I was not able to find a way to replace the click handler with a link as I commented above. What else do you suggest or do you think I have missed in my tests?

tvdeyen · 2024-11-25T16:16:15Z

We definitely still need these table rows to be clickable as that is essentially the backbone of the new admin flow.

Technically we do not need table rows to be clickable in order for the new admin flow to work. Clicking links inside the data cells should open modals or links, not clicking the row itself. It is not possible to wrap a TR in a A in HTML for good reasons.

Consider a table row having multiple "actions" inside (let's say the promotions table has a link to edit the promotion itself and a promotion code, if clicked, visits the promotion codes list). How do we make this possible if only one click action is available for a whole row?

Table rows either redirect to individual edit pages or open modals. I was not able to find a way to replace the click handler with a link as I commented above. What else do you suggest or do you think I have missed in my tests?

Thanks to your work we now have links in all table rows. So we should have the same UX as before, but on the HTML elements that are accessibly and semantically the correct elements. The click events inside the cells should bubble to the handler, so the click handler should still do its work. Ideally the row event handler would only handle modals and nothing else, but this is something to tackle later.

Sorry for all the push back. I know that neither you nor I have been involved in designing or developing the new admin. We now have the burden to make it as maintainable as possible while still getting it done.

I appreciate all the work you put into it, but please bear with we that I am looking at it from a maintainers prospective.

Thanks 🙏🏻

MadelineCollier · 2024-11-25T16:46:49Z

Sorry for all the push back. I know that neither you nor I have been involved in designing or developing the new admin. We now have the burden to make it as maintainable as possible while still getting it done.

I appreciate all the work you put into it, but please bear with we that I am looking at it from a maintainers prospective.

No worries, always happy to work through these hurdles in discussion!

I agree that the table rows should allow for multiple links/actions inside of each row (as in your promotion codes list example), and currently this is what we have. I also agree that having one overarching link would not be good. Currently the click handler will not fire if the user clicks on any of these elements: a,select,textarea,button,input,summary which allows for multiple links (the top level linking on the <tr>, handled by the click handler, and the lower level links on the <td> which are just plain <a> tags.)

We can remove the <tr> click handler, but that would mean that any components inside of the <tr> that are not links eg:

Will now be dead space that is un-clickable. I think this would make the new admin flow quite janky and confusing for users, as they would need to know which parts to click (some of the <td>s are links, while some are not). The solution to this would be to wrap every single <td> across the whole of the new admin in an additional link_to. That sounds like a lot of work. We could DRY that up by doing so at the render_data_cell level (which is the approach i took for cells that are passed as symbols.

However I think that would involve more bad code as we would then need to be checking already rendered component html (in string form) for whether it contains any of these elements: a,select,textarea,button,input,summary in order to ensure we don't double link wrap the content of the <td> and cancel out the intended behaviour that was defined on the column level.

What do you think about the above challenges? Do you see a third option I don't?

tvdeyen · 2024-11-25T18:13:38Z

All of these are solvable by using less code and more semantics :)

We should make linked text inside of the table row visually stand out. Currently there is extra CSS code that makes the links not appear as links. This is bad for a11y and easily be solved by letting the browser do what a browser does: Visually indicate the link as an actionable item (usually by underlining the text or by giving it different color)
Not all components need do be clickable. It does not make sense to click a "complete" pill and assume the edit order link to open. It would make much more sense to filter the table by this state instead. But it is totally ok that a table cell just contains text. It is a list of information and some of the information can be clicked.
We could add a link on all "name"/"number"/"primary indicator" columns, if not already the case.

I think we should not overcomplicate things. The browser, HTML and CSS provide us with simple, semantic, established and accessible tools that all do not require more, but less code.

The web can be reduced to two simple UX patterns: links and forms. It's that simple.

MadelineCollier · 2024-11-26T08:32:32Z

That sounds reasonable @tvdeyen I can take a look at this today. I'll do a pass over all the primary identifiers (most of which should already be links), swap out the content_tag for link_to, and address the spec failures and we should be good.

kennyadsl

Thansk @MadelineCollier, I marked some other occurrences of content_tag :a, let's please fix them, unless there's a reason to keep them as they are.

promotions/lib/components/admin/solidus_promotions/promotion_categories/index/component.rb

legacy_promotions/lib/components/admin/solidus_admin/promotion_categories/index/component.rb

All over the new admin we use divs or simply text to display the resource and rely on a complex Stimulus click event handler to open the edit route. This is not semantic at all and makes it necessary to use a fully JS enabled browser under all circumstances to test the new admin interface which leads to flaky and slow tests. We should use links instead and probably remove the stimulus click event handler. Also, links are not visually highlighted, which leads to a11y issues. So, this commit adds underlines to links in table rows. solidusio#5944

Previously, when provided a symbol, the admin table component would simply call `data.public_send(cell)` (with cell being the symbol). This would result in a basic string as the content of the table row. We want to stop relying on the additional Stimulus click handlers that captured those plain text clicks and redirected the users to wherever the `row_url` location was. Now, the `render_data_cell` method handles wrapping the plain text in a link, with the href being the `row_url` location. Symbols can still be provided as columns and the component takes care of generating the proper semantic UI elements for us.

MadelineCollier · 2024-11-27T12:13:37Z

Thanks for catching those @kennyadsl, fixed! Still having some trouble with the specs. It seems like now that the rowClicked handler isn't in charge anymore, the links aren't preserving the query params (and many specs assert that they should be preserved even when opening/closing modals). Any ideas on how to fix this behaviour? We won't know what the ransack search params are at time of render as far as I can tell. I can also remove those spec assertions if we think its reasonable that if you open a modal to edit a table row, the query params are allowed to be lost.

kennyadsl · 2024-11-27T12:17:00Z

I can't recall exactly what was the need for that requirement. Is there a PR linked to that failing specs that we can reach with a git blame?

MadelineCollier · 2024-11-27T14:00:58Z

I can't recall exactly what was the need for that requirement. Is there a PR linked to that failing specs that we can reach with a git blame?

No blames that I found were particularly helpful as the specs were generally just added as part of the development of the new admin. It makes sense that where possible we’d want to retain the filters/searches that users have inputted, but I can’t find a good way of doing that for all of our new links. This largely only comes into play on edit actions triggered by table link clicks.

kennyadsl · 2024-11-27T14:47:24Z

Yes, you are right. It was probably because when we search for something, edit an item on the row and close the modal, we want to keep the existing selection of the table. cc'ing @elia and @rainerdema to see if we have more details here.

I think we should keep this behavior, as the alternative is a very bad UX where you need to filter again if you want to edit multiple items in the same table after a specific filtering.

MadelineCollier · 2024-11-27T15:38:29Z

Ok that makes sense as a requirement. My only thought is that this was possible before due to the fact that the rowClicked method has full context of what has been searched. Whereas links that are rendered on page load will not have this context. @tvdeyen any ideas on this one? I think we have taken care of all the other points in your original request, but the specs failing due to search params not being persisted is a decent blocker for now.

MadelineCollier · 2024-11-28T13:39:48Z

Ok guys, I have been plugging away at the specs, and while the consistent blocker is the missing query params, another issue has come up that relates to the rowClicked handler.

See it in it's original form:

  rowClicked(event) {
    // If the user clicked on a link, button, input or summary, skip the row url visit
    if (event.target.closest("td").contains(event.target.closest("a,select,textarea,button,input,summary"))) return

    if (this.modeValue === "batch") {
      this.toggleCheckbox(event.currentTarget)
    } else {
      const url = new URL(event.params.url, "http://dummy.com")
      const params = new URLSearchParams(url.search)
      const frameId = params.get('_turbo_frame')
      const frame = frameId ? { frame: frameId } : {}
      // remove the custom _turbo_frame param from url search:
      params.delete('_turbo_frame')
      url.search = params.toString()

      window.Turbo.visit(url.pathname + url.search, frame)
    }
  }

The deletion of the _turbo_frame param turns out to be important for some of our admin pages. Not doing this results in some of our admin edit modals succesfully submitting, updating the given model, but not closing the modal. So the turbostream refresh happens, but the target is still the edit modal, and the page is re-rendered with it open. We could redirect back to the index page but this would clear the query params, and also kind of defeat the purpose of using turboframe modals to begin with. There is discussion around the ability to override the target from the server response here, but nothing solid yet that I have seen in examples.

At this point it feels like 1 step forward, 3 steps back. It would be nice to hear from @elia and @rainerdema to see if this is a path (more semantic UI links instead of stimulus handlers) that has already been explored. This would explain why the rowClicked handler exists today, as it looks like it currently serves two purposes that plain links can't (as far as I have seen in my tests). @tvdeyen I can definitely still appreciate the maintenance angle for the original issue/request to change things up, but hesitate to spend another week on this while so much of the admin remains unfinished and my availability on the project winds down. @kennyadsl Also curious to hear your thoughts on the pros/cons of this.

tvdeyen · 2024-11-28T13:58:30Z

A tricky problem we faced in Alchemy as well. It has been solved by responding with a proper 422 on errors, so that the modal gets redisplayed and properly closed via a redirect from the server. Query parameters are preserved by passing around the ransack parameters.

All that said, I know that this is a tough problem to solve and I was under the impression it can be solved in the new admin.

Unfortunately there is much more involved to fix this issue.

I can totally understand if you want to table this for now.

Maybe I find some time in the future to contribute some of the code we built for Alchemy to Solidus.

Sorry for these circumstances

MadelineCollier · 2024-11-29T10:49:44Z

Ok, in that case, with your blessing @tvdeyen I’ll table this PR for now and get back to the other admin work. Hopefully the learnings from this PR and comment thread can inform the issue whenever we choose to pick it up in the future.

tvdeyen · 2024-12-05T07:08:47Z

Hey @MadelineCollier can we try to just extract 6bd7f5e from this PR? So that we have the links in place? It should work just fine with the row click event handler.

MadelineCollier · 2024-12-05T11:22:31Z

Hey @MadelineCollier can we try to just extract 6bd7f5e from this PR? So that we have the links in place? It should work just fine with the row click event handler.

The links don't work with the specs as they blow away the query params, something which the specs expect to be present after clicking through. Since we still haven't solved that (without javascript) I think we'll need to keep it as is until we have a solution to preserve those params.

tvdeyen · 2024-12-05T15:41:32Z

Ok, I figured it out. A combination of target="_top" on the turbo_frame_tag of the modals form and a proper redirect instead of the turbo_frame refresh in the controllers works as expected. Actually what one would do in regular CRUD apps as well. I will prepare a PR soon.

tvdeyen · 2024-12-05T17:58:40Z

@MadelineCollier This is my current draft PR #6014
Love to have your feedback

MadelineCollier requested a review from a team as a code owner November 25, 2024 14:20

MadelineCollier requested a review from tvdeyen November 25, 2024 14:20

github-actions bot added changelog:solidus_admin changelog:solidus_legacy_promotions Changes to the solidus_legacy_promotions gem changelog:solidus_promotions Changes to the solidus_promotions gem labels Nov 25, 2024

kennyadsl reviewed Nov 25, 2024

View reviewed changes

tvdeyen reviewed Nov 25, 2024

View reviewed changes

MadelineCollier force-pushed the chore/admin-semantic-table-rows branch 2 times, most recently from 392acbd to 7e96eae Compare November 27, 2024 10:52

kennyadsl reviewed Nov 27, 2024

View reviewed changes

MadelineCollier added 2 commits November 27, 2024 13:07

MadelineCollier force-pushed the chore/admin-semantic-table-rows branch from 7e96eae to 7e8a645 Compare November 27, 2024 12:09

wip specs

111c932

MadelineCollier marked this pull request as draft November 28, 2024 13:40

MadelineCollier closed this Nov 29, 2024

tvdeyen mentioned this pull request Dec 5, 2024

[Admin][Users]Add new admin store_credits show page #5978

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Chore/admin semantic table rows #5950

Chore/admin semantic table rows #5950

MadelineCollier commented Nov 25, 2024 •

edited

Loading

kennyadsl left a comment

tvdeyen left a comment

tvdeyen Nov 25, 2024

MadelineCollier Nov 25, 2024

tvdeyen Nov 25, 2024

tvdeyen Nov 25, 2024

tvdeyen Nov 25, 2024

MadelineCollier commented Nov 25, 2024

tvdeyen commented Nov 25, 2024

MadelineCollier commented Nov 25, 2024

tvdeyen commented Nov 25, 2024 •

edited

Loading

MadelineCollier commented Nov 26, 2024

kennyadsl left a comment

MadelineCollier commented Nov 27, 2024

kennyadsl commented Nov 27, 2024

MadelineCollier commented Nov 27, 2024 •

edited

Loading

kennyadsl commented Nov 27, 2024

MadelineCollier commented Nov 27, 2024

MadelineCollier commented Nov 28, 2024

tvdeyen commented Nov 28, 2024

MadelineCollier commented Nov 29, 2024

tvdeyen commented Dec 5, 2024

MadelineCollier commented Dec 5, 2024 •

edited

Loading

tvdeyen commented Dec 5, 2024

tvdeyen commented Dec 5, 2024

	cell = content_tag :a, data.public_send(cell), href: url if cell.is_a?(Symbol)
	cell = link_to(data.public_send(cell), url) if cell.is_a?(Symbol)

Chore/admin semantic table rows #5950

Chore/admin semantic table rows #5950

Conversation

MadelineCollier commented Nov 25, 2024 • edited Loading

Summary

Before:

After:

Checklist

kennyadsl left a comment

Choose a reason for hiding this comment

tvdeyen left a comment

Choose a reason for hiding this comment

tvdeyen Nov 25, 2024

Choose a reason for hiding this comment

MadelineCollier Nov 25, 2024

Choose a reason for hiding this comment

tvdeyen Nov 25, 2024

Choose a reason for hiding this comment

tvdeyen Nov 25, 2024

Choose a reason for hiding this comment

tvdeyen Nov 25, 2024

Choose a reason for hiding this comment

MadelineCollier commented Nov 25, 2024

tvdeyen commented Nov 25, 2024

MadelineCollier commented Nov 25, 2024

tvdeyen commented Nov 25, 2024 • edited Loading

MadelineCollier commented Nov 26, 2024

kennyadsl left a comment

Choose a reason for hiding this comment

MadelineCollier commented Nov 27, 2024

kennyadsl commented Nov 27, 2024

MadelineCollier commented Nov 27, 2024 • edited Loading

kennyadsl commented Nov 27, 2024

MadelineCollier commented Nov 27, 2024

MadelineCollier commented Nov 28, 2024

tvdeyen commented Nov 28, 2024

MadelineCollier commented Nov 29, 2024

tvdeyen commented Dec 5, 2024

MadelineCollier commented Dec 5, 2024 • edited Loading

tvdeyen commented Dec 5, 2024

tvdeyen commented Dec 5, 2024

MadelineCollier commented Nov 25, 2024 •

edited

Loading

tvdeyen commented Nov 25, 2024 •

edited

Loading

MadelineCollier commented Nov 27, 2024 •

edited

Loading

MadelineCollier commented Dec 5, 2024 •

edited

Loading