Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(app-collection): systematically add linked name and "Go to details" cells #1679

Closed
wants to merge 1 commit into from
Closed

feat(app-collection): systematically add linked name and "Go to details" cells #1679

wants to merge 1 commit into from

Conversation

kleinfreund
Copy link
Contributor

@kleinfreund kleinfreund commented Oct 26, 2023

Changes

Systematically adds a “name” cell to AppCollections. The name cell will always be linked as long as either a summary or detail route is provided.

Systematically adds a “Go to details” link to AppCollections that have a summary route. This is intended as the primary quick interaction to go from list to detail view when a summary view exists for that list view. This becomes relevant because the default row click interaction for list views with a summary view is to open the summary view (and no longer the detail view).

Relates to #1681.

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

Notes & motivation

This PR provides AppCollection with access to both the detail and summary route (if applicable). This is intended to move the responsibility for some pretty important interactions from users of AppCollection into AppCollection itself:

  • Row-click interactions. They are now conditional depending on the type of AppCollection: a row-click can go to either a summary route (if provided) or a detail route.
  • Linked name cells. Each use of AppCollection should have a linked name cell (unless there is neither a detail or summary view associated with it).
  • "Go to details" cell. On uses of AppCollection that have a summary, we want to add a "Go to details" cell that makes the navigation to the detail view available in one click and obvious.

I think these items should be handled more systematically than they currently do. It’s easy to forget adding one of these things on the user side of AppCollection.

As an additional bonus, it allow us to move the three-dot "More" dropdown menu code into AppCollection (in #1680) to avoid repeating it in the list view code.

@kleinfreund kleinfreund self-assigned this Oct 26, 2023
@netlify
Copy link

netlify bot commented Oct 26, 2023

Deploy Preview for kuma-gui ready!

Name Link
🔨 Latest commit 5aff66d
🔍 Latest deploy log https://app.netlify.com/sites/kuma-gui/deploys/654cf29a3508a70008f4a19f
😎 Deploy Preview https://deploy-preview-1679--kuma-gui.netlify.app/gui
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

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

Copy link
Contributor

@johncowen johncowen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I took a quick look at this

Overall, I think we should wait until we have feedback from design to do this. From what I understand I don't believe this is stopping us from adding summary views across the application but only making it so you can get to them via a single click i.e. this could be seen as an improvement to the feature (once the feature is in)

Additionally I'm not even sure if we should do the shift-click thing from what I can see that overwrites the default behaviour of my browser which uses shift-click to open the link in a new window.


Honestly, it kinda feels like adding an additional link after the existing 'name' link would probably be fine here. This is what you would do if you were doing this normally. You would just add a new link into the slot.

Adding all this extra stuff to AppCollection is overloading the component with things about trays, detail views etc etc.

AppCollection is supposed to be as simple as a table as we can get.


I wanted to leave some comment here asap seeing as you asked me to, but I still think we should:

  1. Wait a little more for design
  2. Not be too hasty using the shift-click thing, I might have misunderstood in our GUI sync but I think we felt that this would be a little "power user like" anyway.

Happy to catch up on this one first thing tomorrow, but please keep working on getting the summary views in in the meantime this PR shouldn't block any of that I don't think (let me know if I'm wrong)

@kleinfreund
Copy link
Contributor Author

I’ll wait with this PR a little bit mainly so I know which direction #1675 will go in. I’ll likely pull the Shift-Click mechanism out of this. I still would like to add this, but it’s less relevant with the addition of the "Go to details" link.

I plan to integrate a couple of things into AppCollection because they benefit from being abstracted away and into it. This avoids duplicating all sorts of things (the actions dropdown, the "Go to details" link we’re adding, etc.).

That’s exactly what AppCollection should be for.

Honestly, it kinda feels like adding an additional link after the existing 'name' link would probably be fine here. This is what you would do if you were doing this normally. You would just add a new link into the slot.

This is very repetitive and not systematic. We really want this to be systematic so we have less chance of messing it up.

AppCollection is supposed to be as simple as a table as we can get.

Doesn’t mean it shouldn’t help us where it’s the best tool for the job.

@kleinfreund kleinfreund changed the title feat(app-collection): shift-click to open detail view feat(app-collection): systematically add linked name and "Go to details" cells Oct 31, 2023
@johncowen
Copy link
Contributor

johncowen commented Oct 31, 2023

I haven't taken a look at this since we've removed the shift-click thing but wanted to say:

This is very repetitive and not systematic. We really want this to be systematic so we have less chance of messing it up.

The biggest problem we had with our old DataOverview component was, over time people had just added more and more things to it in order to avoid being repetitive and trying to be systematic. The biggest problem with KTable is that over time people had just added more and more things to it in the same way.

Where we've ended up with both of those components is extremely overloaded, difficult to manage components that in some cases tried to abstract the entire application into one component (bit of an exaggeration but you know what I mean). I want to take care to try to avoid returning to that.

Now, these things maybe seem simple, but they are sometimes hard to get right, and its is often a fine balance, but sometimes its just easier and more obvious, and in the longer term more flexible, to just use a <a href="/detail-view">Detail</a> link. Links are pretty hard to mess up, whereas the rapidly growing code inside AppCollection isn't.

As I said its a fine balance, and sometimes hard to get right, but we should try, and with that being said I'll take a look at this again, but as it stood previously my overarching thought was, "why aren't we just using a link?"

@kleinfreund
Copy link
Contributor Author

The biggest problem we had with our old DataOverview component was, over time people had just added more and more things to it in order to avoid being repetitive and trying to be systematic.

Yeah but the abstraction of some (!) of thisstuff was actually a good part of DataOverview that we dropped with AppCollection. We currently repeat the exact same dropdown code over about 10 list views. That’s just not great. The same applies to various additions like the "Go to details" cell or the linked name cell. We shouldn’t need to re-implement those every time.

…ls" cells

Systematically adds a “name” cell to AppCollections. The name cell will always be linked as long as either a summary or detail route is provided.

Systematically adds a “Go to details” link to AppCollections that have a summary route. This is intended as the primary quick interaction to go from list to detail view when a summary view exists for that list view. This becomes relevant because the default row click interaction for list views with a summary view is to open the summary view (and no longer the detail view).

Signed-off-by: Philipp Rudloff <[email protected]>
@lahabana lahabana marked this pull request as draft November 13, 2023 10:12
@lahabana
Copy link
Contributor

Draft until we figured the UX a little more.

@kleinfreund
Copy link
Contributor Author

Closing this for the time being. If I pick this up again, it’ll likely happen via #1680.

@kleinfreund kleinfreund removed their assignment Nov 23, 2023
@kleinfreund kleinfreund deleted the feat/shift-click-to-open-detail-view branch April 17, 2024 12:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants