Skip to content

feat: add middle click support for workspace rows #9834

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

Merged
merged 12 commits into from
Sep 25, 2023
Merged

Conversation

Parkreiner
Copy link
Member

@Parkreiner Parkreiner commented Sep 22, 2023

Closes #9749

This seemed really easy at first – and getting the runtime logic updated was no problem – but it quickly turned into an ordeal because of all the TypeScript type parameters I had to juggle.

Changes

  • Adds middle-click + Cmd-/Ctrl-Click support to workspace rows, to make it easy to open workspace pages in new tabs
  • Updates useClickable hook to expose a ref. This can be used in effects, but right now, the main point is to simulate a click for the element the props are attached to
  • Updates useClickableTableRow to support onClick, onDoubleClick and onAuxClick callbacks (onAuxClick was needed for middle-clicks, so I figured I'd add support for everything)

Out-of-scope changes for later

  • Right now, the middle-click functionality isn't in place for the other components using the useClickableTableRow hook, but I tried to make it as easy as possible to add that functionality later, if they need it

// Awkward type definition (the hover preview in VS Code isn't great, either),
// but this basically takes all click props from TableRowProps, but makes
// onClick required, and adds an optional onMiddleClick
type UseClickableTableRowConfig = {
Copy link
Member Author

@Parkreiner Parkreiner Sep 25, 2023

Choose a reason for hiding this comment

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

The more I think about this, the more I think I should probably update this with this "less-clever"/"actually maintainable" type:

type UseClickableRowConfig = {
  onClick: MouseEventHandler<HTMLTableRowElement>;
  onAuxClick?: MouseEventHandler<HTMLTableRowElement>;
  onDoubleClick?: MouseEventHandler<HTMLTableRowElement>;
  onMiddleClick?: MouseEventHandler<HTMLTableRowElement>;
};

You lose the explicit connection to the underlying TableRowProps type, so any changes to it might not immediately get flagged at the compiler level, but it's way more readable and gives you better information when you hover over the type in VS Code

@Parkreiner Parkreiner marked this pull request as ready for review September 25, 2023 13:51
@Parkreiner Parkreiner requested review from a team and BrunoQuaresma and removed request for a team September 25, 2023 13:51
Copy link
Collaborator

@BrunoQuaresma BrunoQuaresma left a comment

Choose a reason for hiding this comment

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

Good work, I think it is a good solution for now but I have a few thoughts:

  • Wondering how accessible it is since it only supports middle click but would not work with right click > open in new tab
  • I'm 0wondering if we should start a conversation to:
    • Get away from tables so we could wrap the rows as an ""
    • Make only the name column be clickable so we could achieve that easily... but not sure what other folks would think about that 🤔

@Parkreiner
Copy link
Member Author

Parkreiner commented Sep 25, 2023

Good work, I think it is a good solution for now but I have a few thoughts:

  • Wondering how accessible it is since it only supports middle click but would not work with right click > open in new tab

  • I'm 0wondering if we should start a conversation to:

    • Get away from tables so we could wrap the rows as an ""
    • Make only the name column be clickable so we could achieve that easily... but not sure what other folks would think about that 🤔

Yeah, good point. I feel like that would require some changes to how the components are designed – namely, we would have to update the semantics and ARIA values to make the rows into links, rather than buttons.

But I do also think the rows should probably change a little bit. I don't know what features we could add down the line that would warrant having different functionality for each cell, but at the very least, I feel like the checkbox column can be separated and can be set up as a big, implicit checkbox. So even if someone accidentally misses the visible checkbox, just clicking the area will still make the check go through. That would help the a11y a little bit

@Parkreiner Parkreiner merged commit 3757005 into main Sep 25, 2023
@Parkreiner Parkreiner deleted the mes/middle-click branch September 25, 2023 15:32
@github-actions github-actions bot locked and limited conversation to collaborators Sep 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feature: Be possible to "middle click" on a workspace row to open in a new tab
2 participants