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

Allow links in row actions and more actions dropdowns #2751

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

david-crespo
Copy link
Collaborator

Closes #1855

It's more noisy than it needs to be because I also cleaned up some duplicated logic that didn't need to exist — notice there are net deletes despite the increase in functionality. I think I would like to convert RowActions to taking elements directly as children as well instead of using these action config objects we have to memoize, but that would be too much churn for this PR: we have row actions in like 20 files.

Copy link

vercel bot commented Mar 11, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
console ✅ Ready (Inspect) Visit Preview Mar 12, 2025 0:08am

onActivate() {
navigate(pb.serialConsole({ project, instance: instance.name }))
},
to: pb.serialConsole({ project, instance: instance.name }),
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is the narrow change asked for on behalf of an external user in #1855. This allows serial console to be opened in a new tab from both instance list row actions and instance detail "more actions".

<MoreActionsMenu label="Router actions" actions={actions} />
<MoreActionsMenu label="Router actions">
<CopyIdItem id={routerData.id} />
</MoreActionsMenu>
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

net negative lines speak for themselves I think

})}
className="destructive"
/>
</MoreActionsMenu>
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This shows why the children approach is nice. There's no config object where you then elsewhere have to decide what it renders. You just render those things directly.

@david-crespo
Copy link
Collaborator Author

david-crespo commented Mar 11, 2025

Claude caught a real bug. A bargain at 2.8 cents.

image

The bug causes this button to do nothing because it's rendering what should be a link as a button.

2025-03-11-claude-link-bug.mp4

Reassuringly it was also caught be our e2e tests. Still, nice one Claude!

image

/>
)
)}
</MoreActionsMenu>
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is the one spot where we are adding some noise, because this is the one spot where we were passing the same set of action config objects to RowActions in one place (instance list) and MoreActionsMenu in another (here, instance detail). Because MoreActionsMenu has been converted to take children directly, we have to do the conversion to elements here. Maybe this should be extracted since it's the same as what RowActions does internally....

@@ -66,7 +78,6 @@ type RowActionsProps = {
export const RowActions = ({ id, copyIdLabel = 'Copy ID', actions }: RowActionsProps) => {
return (
<DropdownMenu.Root>
{/* TODO: This name should not suck; future us, make it so! */}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This comment is from May 2022 #883 and refers to the aria label "Row actions", which tbh I don't see anything wrong with. That's what they are.

@david-crespo
Copy link
Collaborator Author

david-crespo commented Mar 11, 2025

Tolerable external link icon, @benjaminleonard? (I changed it to "about this metric" — somehow "about metric" felt like a typo.)

image

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.

Allow real links in more actions menu
1 participant