feat: adds row selection ability to app collection

[kleinfreund]

Changes

refactor: makes AppCollection generic

feat: adds support for marking a selected row

Adds support for marking a selected row with AppCollection based on a function returning a boolean provided to props.isSelectedRow.

Adds row:click event listeners to all consumers of AppCollection so that clicking a row still navigates to corresponding detail view.

Resolves #1570.

Signed-off-by: Philipp Rudloff [email protected]

Approach

Current approach

Table rows always have a “name” cell in first position.

<template #name="{ row: item }">
  <RouterLink
    :to="{
      name: 'service-detail-view',
      params: {
        service: item.name,
      },
    }"
  >
    {{ item.name }}
  </RouterLink>
</template>

On KTable row:click, we run the following code to implement “click row to go to detail view” in AppCollection:

const click = (e: MouseEvent) => {
  const $tr = (e.target as HTMLElement).closest('tr')
  if ($tr) {
    const $a = $tr.querySelector('a')
    if ($a !== null) {
      $a.click()
    }
  }
}

Introducing summary trays

We’re in the process of introducing summary trays to all list views. Those trays will be invoked by clicking a table row. For this purpose, #1601 changes the handling of the KTable row:click handler to re-emit a row:click event instead of always trying to click the first link in a table row (see code snippet above).

function handleRowClickEvent(_event: unknown, row: Row) {
  emit('row:click', row)
}

This transfers the responsibility of handling the “click row to go to detail view” functionality to the list views.

<AppCollection
  @row:click="router.push({
    name: 'service-detail-view',
    params: {
      service: $event.name,
    },
  })"
>

This allows us to implement summary trays one by one by changing an AppCollection’s row:click handling:

<AppCollection
  @row:click="router.push({
    name: 'service-summary-view', // Note the different route name.
    params: {
      service: $event.name,
    },
  })"
>

Changing “name” cell links

As part of introducing summary trays, we will either (1) maintain that the “name” cell links to the corresponding detail view or (2) remove the link to detail views (and only rely on the “More” dropdown menu and its “View details” action).

In scenario (1), nothing needs to change because all list views already have links to the right detail view in their “name” cells.

In scenario (2), we would remove the links from the “name” cells.

[netlify]

✅ Deploy Preview for kuma-gui ready!

Name	Link
🔨 Latest commit	cdb8145
🔍 Latest deploy log	https://app.netlify.com/sites/kuma-gui/deploys/65292f171293920008b1699a
😎 Deploy Preview	https://deploy-preview-1601--kuma-gui.netlify.app/gui
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site configuration.

[johncowen]

I was wondering about the amount of code used here to do this, also considering we want to use the state in the URL. I wasn't sure if I was missing something so I had a quick look here:

master...johncowen:kuma-gui:scratch/tray

..which seems to be the feature with less code and using the state from the URL instead of maintaining our own separate one.

Which in essence is using the name segment in the URL and seeing if its the same as the data used for the row, and if so applying the is-selected class. Guessing I might be missing something though? lmk.

---

A few other general UX things:

• I don't think we should automatically select the first item in the table. When I come to the listing I should see the entire listing with no tray. Only when I choose to click to see the tray should I see it.
• I think the entire row, including the "name of the thing" should show the summary tray, if you want to see the detail page you should use the action menu on the right. It would seem strange to me that the entire row apart from the letters of the "name of thing" show the summary, but it I accidentally click on a bit of the "name of the thing" I jump to a completely different page. That would be surprising to me. I can definitely imagine myself clicking up and down the list to compare "things" and accidentally clicking on a bit of the "name of the thing" in doing so.

Let me know what you think about those ^ two UX things also.

[kleinfreund]

@johncowen I’ve changed this partially now to work with a props.isSelectedRow. I don’t think the entire row attributes function should be externalized. This does avoid us having to map names to IDs for this purposes and is indeed a little simpler.

• I don't think we should automatically select the first item in the table. When I come to the listing I should see the entire listing with no tray. Only when I choose to click to see the tray should I see it.

I agree and this isn’t happening with these changes anymore.

• I think the entire row, including the "name of the thing" should show the summary tray, if you want to see the detail page you should use the action menu on the right. It would seem strange to me that the entire row apart from the letters of the "name of thing" show the summary, but it I accidentally click on a bit of the "name of the thing" I jump to a completely different page. That would be surprising to me. I can definitely imagine myself clicking up and down the list to compare "things" and accidentally clicking on a bit of the "name of the thing" in doing so.

That’s contentious and I want to discuss it in a bigger forum mainly because it’s a feature we’ve established for a long time now. Taking it away should be treated carefully.

Note that this is really much more of a problem because one can’t really tell what’s a link in our application. This would be pretty much a non issue with underlined links.

[johncowen]

I don’t think the entire row attributes function should be externalized.

I'm still pretty keen on just using/passing though the rowAttrs function that already exists on KTable. What if we wanted to add a class based on say whether the thing in the row was in a warning state? There are probably other things that I haven't immediately thought of, but it didn't take me long to think of the reasonably likely warning use case. (umm also, we have soft/delayed deletes in places which I'm not sure we expose currently, but thats another use case for marking a row differently to others)

Alternatively, for those other use cases would we keep on adding more and more props instead of using rowAttrs?

Also, I think that using rowAttrs is the closest we'll get to just being able to do:

<Collection>
  <template v-for="item in items">
    <CollectionItem :class="{
       'is-selected': route.name === item.name,
       'warning': item.warnings.length > 0
    }">...</CollectionItem>
  </template>
</Collection>

seeing as its almost the same as:

<Collection
  row-attrs=(item) => ({
    'is-selected': route.name === item.name,
    'warning': item.warnings.length > 0
  }}
>
  
</Collection>

Lastly I'm still really keen on keeping AppCollection i.e. a presentational Table, as simple and presentational as possible IMO AppCollection is already overloaded with too much functionality that we would need if we wanted a collection of things that wasn't rendered as a table.

What's your rationale for preferring the extra prop over passing through rowAttrs? I can possibly think of one I think but I don't think its that relevant for us.

---

I agree and this isn’t happening with these changes anymore.

👍

That’s contentious and I want to discuss it in a bigger forum mainly because it’s a feature we’ve established for a long time now. Taking it away should be treated carefully.

Ok, thats sounds good, but also worth noting we've already taken it away.

Note that this is really much more of a problem because one can’t really tell what’s a link in our application. This would be pretty much a non issue with underlined links.

It would still be an issue I think, I would expect the entire row to the same thing whether I click on the name/identifier for the thing or elsewhere in the row (unless its clearly a different entity in the row i.e. a zone in a service row, in which case we currently color that bright blue). I think the fact that all rows now have an action menu that have a clear "View details" item also helps as that one clearly say to me "you are going to navigate to the detail here, not a summary"

Note GitHub does a thing in areas where you click the name of the thing and you open a summary, then the title inside the summary links you to the detail, which also makes a tonne of sense to me.

[kleinfreund]

Alternatively, for those other use cases would we keep on adding more and more props instead of using rowAttrs?

Yes. The we can’t mess up the class names and structure of row-attrs. I’m not convinced this needs changing.

[johncowen]

We spoke about this at length offline and the crux of the discussion seems to revolve around two options:

1. The text in the table row/button links to a different page to the background of the table row/button.
2. The text in the table row/button links to the same page as the background of the table row/button.

If we go with option 1, I think this is good but we probably want to also remove the links from the rows and just use the JS click events.
If we go with option 2, we should probably remove the JS click events from here and just stick with the "find the first link in row/button to use for the link"

If this needs to go in sooner than we can have a "discussion in a bigger forum" then I would rather not add the new JS events and just use the links as it is now in order to get this approved now and then potentially (or potentially not) add the event handlers later. (Even if we knew we wanted to keep the events now, we should probably remove the anchors here so we aren't doubling these things up).

Personally, I've had a look at the UX of some other popular websites and I'm yet to see one where the "name of the thing"/"text of the button" links to a different place than the background of the row/button, but I'd be happy to get the options for the decision clearly written down so we can make a decision in that bigger forum and then go with it.

Hope thats ok.

[kleinfreund]

@johncowen I still don’t understand why this PR is being blocked. I explained (multiple times) how I don’t intend to keep the mechanism of programmatically clicking the first link in a table row regardless of the outcome of said conversation (i.e. whether to keep linking to a detail view or whether to drop that functionality).

I want to replace this mechanism because it’s problematic: Add a new row entry with a link in the first column and the mechanism will immediately break. This should simply be impossible. The changes in this PR ensures that this can’t happen regardless of row content.

This PR is blocking me on #1609. Please finish reviewing it.

[kleinfreund]

Specifically for the scenario where we don’t link to detail views from the a name column anymore, there isn’t actually any point to have a link be there in the first place. That makes me doubly say we shouldn’t use a "click first link in table row" mechanism.

[johncowen]

Oh hey, sorry I thought we'd finished up our convo offline, and you'd asked me to add something here so I did.

The "blocking" thing: we are reviewing this now i.e. it is progressing. The PR has been up for a comparatively a short period of time, especially as there are two clear opposites as to what we want to do here, and there are barely hours between each comment on this thread, so I wouldn't say it is being "blocked" there is an active discussion. You also said offline that this PR wasn't urgent.

Offline I suggested that writing a MADR for this for a group decision would be a good idea, but you didn't want to do that. I also pointed out offline that I would happily approve this if we leave the link functionality as it is, at least in the short term, i.e. using links to navigate. And then, as a team, if we do decide to have two different actions depending on where you click in the row, we can do something to accommodate that.

---

there isn’t actually any point to have a link be there in the first place

Until we make a decision at least, I would rather stick to using links for navigation, for reasons very similar to those that you yourself pointed out here:

Triggering a click via JavaScript isn’t enough to make the tab behave like a link.
The only thing that works right is that clicking the tab will activate it.

But anything else associated with link functionality doesn’t:

• Right-click doesn’t show any link-specific entries in the context menu like "Open link in new tab"
• Browser shortcuts available for links don’t work (e.g. Ctrl-Click/MouseWheel-Click to open link in a new tab)
• Dragging the link doesn’t work
• Hovering doesn’t show the URL in the browser

#922 (comment)

In the above PR, I changed the code to use links rather than JS events as I agreed with your reasoning, I don't understand why here you are saying the opposite.

If you want to get this PR approved now (i.e. "unblock it") you could remove the JS events (and if we decide we need them later we can add them then). The little branch I posted above proves that it works fine without the JS events, I checked again yesterday when we were chatting and unless I'm missing something (let me know if I am) I don't understand why we need the JS events right now in order to "unblock" #1609.

---

Leaving the "outcome of said conversation" behind then, I definitely can't approve this without further discussion on why we aren't using links for navigation.

I am extra cautious considering this PR initially had us using JS inside the component to maintain an extra copy of state separate to the state in the URL, something which has caused us lots of issues in the past and we are just about rid of those issues. I don't want to rush these things just because you say you are "blocked".

I don’t intend to keep the mechanism of programmatically clicking the first link in a table row regardless of the outcome of said conversation

As it stands now, I don't understand why in a previous situation you gave good arguments for using links to navigate, and then in this situation you seem to have the opposite opinion. Could you explain why in this case you don't want to use links? how is it different from the previous situation?

Add a new row entry with a link in the first column and the mechanism will immediately break.

I don't follow this? Maybe I've misunderstood but if I add a new row entry with the correct link as the first (or tagged) link, how does it break? Do you mean if we add a different link in a row that precedes the link we want to use as the "navigator"?

If you can help me to understand that ^ that would maybe help.

Let's see if we can get something done here today, I'll be responding as soon as possible either here or if you want to continue to the conversation in the thread offline that also works for me.

---

Just incase, I'm fine with the row-attrs vs isSomethingFunction discussion, to some extent I can see why there are circumstances where you would want to have a property vs a classname. Not super important but would it be possible to default the property to a () => false instead of null so the so we are using the same type always? That has the consequence of making the logic further down a bit simpler also.

In the meantime I'm going to take another look at the code here. Let me know on the clarifications/questions above.

[johncowen]

ty!