Adrian/add visualize reset button - #122940
Conversation
|
@cursor review |
github-actions
Bot
added
the
Scope: Frontend
| <Button | ||
| variant="transparent" | ||
| icon={<IconDelete />} | ||
| icon={<IconDelete size="sm" />} |
There was a problem hiding this comment.
Adjusted size to sm to keep consistency w/ the group by one
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit b144110. Configure here.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| <ToolbarVisualizeHeader /> | ||
| {editableColumns.map((column, i) => { | ||
| const visualize = column.column; | ||
| const dragColumnId = editableColumns.length > 1 ? column.id : undefined; | ||
| const label = ( | ||
| <VisualizeLabel | ||
| index={i} | ||
| visualize={visualize} | ||
| onClick={() => toggleVisibility(i)} | ||
| /> | ||
| ); | ||
| const onDelete = | ||
| editableColumns.length > 1 ? () => deleteColumnAtIndex(i) : undefined; | ||
|
|
||
| if (isVisualizeEquation(visualize)) { | ||
| return ( | ||
| <VisualizeEquationInput | ||
| key={column.uniqueId} | ||
| dragColumnId={dragColumnId} | ||
| onDelete={onDelete} | ||
| onReplace={newVisualize => replaceOverlay(i, newVisualize)} | ||
| const isOnlyVisualize = editableColumns.length === 1; | ||
| const canReset = isOnlyVisualize && !isDefaultVisualize(visualize); | ||
| const onDelete = isOnlyVisualize | ||
| ? canReset |
There was a problem hiding this comment.
Bug: Clicking 'Clear Visualize' on a hidden, non-default chart unintentionally makes it visible again because its visibility state is not preserved.
Severity: LOW
Suggested Fix
When resetting the visualization, preserve its current visibility state. When calling replaceOverlay, pass the visible property from the existing visualize object to the new VisualizeFunction constructor, ensuring a hidden chart remains hidden after being cleared. For example: () => replaceOverlay(i, new VisualizeFunction(DEFAULT_VISUALIZATION, {visible: visualize.visible})).
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: static/app/views/explore/toolbar/toolbarVisualize.tsx#L111-L117
Potential issue: When a single, non-default visualization is hidden, the 'Clear
Visualize' button is displayed. Clicking this button calls `replaceOverlay` with a new
`VisualizeFunction` instance using default parameters. The `VisualizeFunction`
constructor defaults the `visible` property to `true` if it is not explicitly provided.
Consequently, clearing a hidden, non-default chart not only resets it to the default
visualization but also unintentionally makes it visible again, ignoring the user's
previous action to hide it.
There was a problem hiding this comment.
I think this is okay, makes sense to me
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 1313040. Configure here.
2 participants
This PR adds a reset button to the
visualizerow the same waygroupByadded in EXP-802. In short, anytime a chart/equation is added or changed, the reset (trash) button will appear, allowing you to reset said chart/equation. This was the case for 2 or more chart/equations, but this PR added that behavior for the 1st equation if that is changed from its defaultcountvalue.Before
After
Addresses EXP-837