-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
feat(cell-action): add external linking as action #95892
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(cell-action): add external linking as action #95892
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👀 ! Two questions:
- In a few places the
isUrl
functionality is checking the feature flag, but sometimes not, is that intentional? - I'm worried that people won't remember to import
handleCellActionFallback
, and this'll result in inconsistent fallback handling. Is it possible to handle the fallback insideCellActions
?
@gggritso I think we would need to refactor how Would it be better to split it into two helper functions (e.g., |
@lzhao-sentry I like that idea! Go for it 👍🏻 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍🏻
@gggritso Apologies, need a re-review. Was showing this to Dora and Dhrumil and they asked for logs to still use the ellipsis trigger since the bold hover trigger doesn't fit well there. I added an extra prop and enum to support this |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Clipboard Copy Triggers Unnecessary Query Update
The Actions.COPY_TO_CLIPBOARD
case was removed from the handleCellAction
switch statement. Copy actions now fall through to the default
case, which calls updateQuery
. This unnecessarily executes query modification logic and browser history navigation, despite updateQuery
handling the copy internally.
static/app/views/discover/table/tableView.tsx#L611-L625
sentry/static/app/views/discover/table/tableView.tsx
Lines 611 to 625 in 8deae8f
} | |
default: { | |
// Some custom perf metrics have units. | |
// These custom perf metrics need to be adjusted to the correct value. | |
let cellValue = value; | |
const unit = tableData?.meta?.units?.[column.name]; | |
if (typeof cellValue === 'number' && unit) { | |
if (Object.keys(SIZE_UNITS).includes(unit)) { | |
// @ts-expect-error TS(7053): Element implicitly has an 'any' type because expre... Remove this comment to see the full error message | |
cellValue *= SIZE_UNITS[unit]; | |
} else if (Object.keys(DURATION_UNITS).includes(unit)) { | |
// @ts-expect-error TS(7053): Element implicitly has an 'any' type because expre... Remove this comment to see the full error message | |
cellValue *= DURATION_UNITS[unit]; | |
} | |
} |
const cellActions = makeCellActions(props); | ||
const {children, column} = props; | ||
|
||
const useCellActionsV2 = organization.features.includes('discover-cell-actions-v2'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Feature Flag Naming Error
The discover-cell-actions-v2
feature flag check is missing the organizations:
prefix, it should be organizations:discover-cell-actions-v2
.
Locations (4)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Logs is the only table to use the ellipsis trigger.
:cryblood:
Changes
Have external linking as a dropdown menu option for
CellAction
. Removed cell actions for some ID linked fields (since it conflicts with the dropdown & should just directly open the link)Video
external.linking.mov