[Metrics][Discover] Add an option to clear the selection in the dropdowns#233776
Conversation
…r-the-selection-in-the-dropdowns
|
Pinging @elastic/obs-ux-infra_services-team (Team:obs-ux-infra_services) |
…r-the-selection-in-the-dropdowns
…r-the-selection-in-the-dropdowns
There was a problem hiding this comment.
Looks good. Left a few nits
On the new package to centralize the ToolbarSelector, I don't have any strong opinions against it. The component appears to be generic enough to be reused outside the Discover/Metrics Experience context, and it may help as more features are added to One Discover. We'll see.
| fields={fields} | ||
| onChange={onDimensionsChange} | ||
| selectedDimensions={dimensions} | ||
| clearDimensionSelection={() => onClearAllDimensions()} |
There was a problem hiding this comment.
For event, let's follow the on[Event] convention, like onChange
| clearDimensionSelection={() => onClearAllDimensions()} | |
| onClear={onClearAllDimensions} |
There was a problem hiding this comment.
Have you checked this suggestion?
There was a problem hiding this comment.
Sorry, missed that. Good point, renamed ✅
| disabled={dimensions.length === 0} | ||
| indices={[indexPattern]} | ||
| timeRange={timeRange} | ||
| clearValuesSelection={() => dispatch(setValueFilters([]))} |
There was a problem hiding this comment.
Same here. Perhaps you could also use the onValuesChange and pass an empty array
| clearValuesSelection={() => dispatch(setValueFilters([]))} | |
| const onClearValues = useCallback(() => onValuesChange([])) | |
| <ValuesSelector | |
| onClear={onClearValues} |
There was a problem hiding this comment.
Have you checked this suggestion? Wdyt?
| return ( | ||
| <FormattedMessage | ||
| id="metricsExperience.breakdownFieldSelector.breakdownFieldButtonLabel" | ||
| id="metricsExperience.dimensionsSelector.breakdownFieldButtonLabel" |
There was a problem hiding this comment.
nice. thanks for fixing this.
| options={options} | ||
| singleSelection={false} | ||
| onChange={handleChange} | ||
| popoverTitle={i18n.translate('metricsExperience.dimensionsSelector.label', { |
There was a problem hiding this comment.
There’s no title in the designs. Do we need one?
There was a problem hiding this comment.
True, I removed it. It was also adding the aria-labels using the title content, and unfortunately, we won't get them now 😞
| ); | ||
| }, [selectedDimensions]); | ||
|
|
||
| const clearAllSection = useMemo(() => { |
There was a problem hiding this comment.
We're repeating this component here and in the values_selector. Should we extract it into a new file and reuse it on both components?
There was a problem hiding this comment.
Good idea! I expected that they would have more differences when I added them. I created a component and reused it 👍
| searchable={true} | ||
| singleSelection={true} | ||
| popoverTitle="My Popover Title" | ||
| clearAllSection={<span>Custom Section</span>} |
There was a problem hiding this comment.
If clearAllSection accepts any component - not necessarily a component that has a selection counter and a clear all button -, should we rename the prop to something more generic but clear enough to tell it will be rendered below the search bar?
There was a problem hiding this comment.
Makes sense, thanks! I also wasn't sure how to call it TBH, I asked GH Copilot and it suggested popoverContentBelowSearch, so I would go with that (this was the best suggestion it gave IMO 😅 )
There was a problem hiding this comment.
maybe searchProps: { append: React.ReactNode } }?
There was a problem hiding this comment.
We already have search props > append in a different context when using search in EuiSelectable 🤔 And it's not directly inside the search, so I think popoverContentBelowSearch describes it well. Or maybe just contentBelowSearch, idk
| fireEvent.click(getByText('B')); | ||
| expect(onChange).toHaveBeenCalled(); | ||
| // Should be called with array of selected options | ||
| expect(Array.isArray(onChange.mock.calls[0][0])).toBe(true); |
There was a problem hiding this comment.
Could we validate the items that were passed to onChange?
eg:
expect(onChange).toHaveBeenCalledWith([
{ label: 'A', value: 'a' },
{ label: 'B', value: 'b' },
]);There was a problem hiding this comment.
Thanks for the suggestions! We have 2 calls, so I used toHaveBeenNthCalledWith and checked both calls 👍
| fireEvent.click(getByTestId('toolbarSelectorTestButton')); | ||
| fireEvent.click(getByText('Option 2')); | ||
| expect(onChange).toHaveBeenCalled(); | ||
| expect(onChange.mock.calls[0][0].label).toBe('Option 2'); |
There was a problem hiding this comment.
Could we validate it like this?
expect(onChange).toHaveBeenCalledWith({ label: 'Option 2', value: 'option2' });
AlexGPlay
left a comment
There was a problem hiding this comment.
discover changes lgtm - code review only
…tion-in-the-dropdowns' of https://github.com/jennypavlova/kibana into 233653-metricsdiscover-add-an-option-to-clear-the-selection-in-the-dropdowns
jennypavlova
left a comment
There was a problem hiding this comment.
@crespocarlos Thank you for reviewing! I believe I addressed all the comments. Can you please take a look again when you have time?
| searchable={true} | ||
| singleSelection={true} | ||
| popoverTitle="My Popover Title" | ||
| clearAllSection={<span>Custom Section</span>} |
There was a problem hiding this comment.
Makes sense, thanks! I also wasn't sure how to call it TBH, I asked GH Copilot and it suggested popoverContentBelowSearch, so I would go with that (this was the best suggestion it gave IMO 😅 )
| fireEvent.click(getByText('B')); | ||
| expect(onChange).toHaveBeenCalled(); | ||
| // Should be called with array of selected options | ||
| expect(Array.isArray(onChange.mock.calls[0][0])).toBe(true); |
There was a problem hiding this comment.
Thanks for the suggestions! We have 2 calls, so I used toHaveBeenNthCalledWith and checked both calls 👍
| fireEvent.click(getByTestId('toolbarSelectorTestButton')); | ||
| fireEvent.click(getByText('Option 2')); | ||
| expect(onChange).toHaveBeenCalled(); | ||
| expect(onChange.mock.calls[0][0].label).toBe('Option 2'); |
| options={options} | ||
| singleSelection={false} | ||
| onChange={handleChange} | ||
| popoverTitle={i18n.translate('metricsExperience.dimensionsSelector.label', { |
There was a problem hiding this comment.
True, I removed it. It was also adding the aria-labels using the title content, and unfortunately, we won't get them now 😞
| ); | ||
| }, [selectedDimensions]); | ||
|
|
||
| const clearAllSection = useMemo(() => { |
There was a problem hiding this comment.
Good idea! I expected that they would have more differences when I added them. I created a component and reused it 👍
…r-the-selection-in-the-dropdowns
…r-the-selection-in-the-dropdowns
davismcphee
left a comment
There was a problem hiding this comment.
Sorry for getting to this late after it was already approved on our end. Data Discovery code changes seem fine to me, just have a question about the dependencies and new package.
|
Just an fyi that I disabled auto merge to give @jennypavlova an opportunity to respond to my comment before it auto merges. |
…r-the-selection-in-the-dropdowns
💚 Build Succeeded
Metrics [docs]Module Count
Public APIs missing comments
Async chunks
Public APIs missing exports
Page load bundle
Unknown metric groupsAPI count
History
|
…owns (elastic#233776) Closes elastic#233653 ## Summary This PR adds an option to clear the selection in the dimension and value selectors. When all dimensions are cleared, it will clear all values as well as those related to the corresponding dimensions. The PR also extracts the toolbar_selector to a shared place and uses it in the metrics grid and the histogram without adding the dependency issue. <img width="1276" height="669" alt="image" src="https://github.com/user-attachments/assets/e83d3338-fbe2-4f87-b2b0-d66ec50b79c9" /> <img width="2328" height="687" alt="image" src="https://github.com/user-attachments/assets/6c13b429-8566-4f8f-aa88-35207f524abf" /> ## How to test - Set the following config to `kibana.dev.yml` ```yml discover.experimental.enabledProfiles: - observability-metrics-data-source-profile metricsExperience.enabled: true ``` - Navigate to Discover and Switch to ESQL mode - Set the space to Observability - Run `FROM metrics-*` search - Clone https://github.com/simianhacker/simian-forge/tree/main - Run ` ./forge --dataset hosts --count 25 --interval 30s` - Check the drop-downs in the metrics grid: https://github.com/user-attachments/assets/77313c81-59fa-41ee-b3f9-60384212cfab - In case of `logs-*` search, for example, the breakdown selector should work as before: <img width="3544" height="1332" alt="image" src="https://github.com/user-attachments/assets/ce33654f-9835-456a-b47f-6a60ed58edd8" /> --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
…owns (elastic#233776) Closes elastic#233653 ## Summary This PR adds an option to clear the selection in the dimension and value selectors. When all dimensions are cleared, it will clear all values as well as those related to the corresponding dimensions. The PR also extracts the toolbar_selector to a shared place and uses it in the metrics grid and the histogram without adding the dependency issue. <img width="1276" height="669" alt="image" src="https://github.com/user-attachments/assets/e83d3338-fbe2-4f87-b2b0-d66ec50b79c9" /> <img width="2328" height="687" alt="image" src="https://github.com/user-attachments/assets/6c13b429-8566-4f8f-aa88-35207f524abf" /> ## How to test - Set the following config to `kibana.dev.yml` ```yml discover.experimental.enabledProfiles: - observability-metrics-data-source-profile metricsExperience.enabled: true ``` - Navigate to Discover and Switch to ESQL mode - Set the space to Observability - Run `FROM metrics-*` search - Clone https://github.com/simianhacker/simian-forge/tree/main - Run ` ./forge --dataset hosts --count 25 --interval 30s` - Check the drop-downs in the metrics grid: https://github.com/user-attachments/assets/77313c81-59fa-41ee-b3f9-60384212cfab - In case of `logs-*` search, for example, the breakdown selector should work as before: <img width="3544" height="1332" alt="image" src="https://github.com/user-attachments/assets/ce33654f-9835-456a-b47f-6a60ed58edd8" /> --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
…owns (elastic#233776) Closes elastic#233653 ## Summary This PR adds an option to clear the selection in the dimension and value selectors. When all dimensions are cleared, it will clear all values as well as those related to the corresponding dimensions. The PR also extracts the toolbar_selector to a shared place and uses it in the metrics grid and the histogram without adding the dependency issue. <img width="1276" height="669" alt="image" src="https://github.com/user-attachments/assets/e83d3338-fbe2-4f87-b2b0-d66ec50b79c9" /> <img width="2328" height="687" alt="image" src="https://github.com/user-attachments/assets/6c13b429-8566-4f8f-aa88-35207f524abf" /> ## How to test - Set the following config to `kibana.dev.yml` ```yml discover.experimental.enabledProfiles: - observability-metrics-data-source-profile metricsExperience.enabled: true ``` - Navigate to Discover and Switch to ESQL mode - Set the space to Observability - Run `FROM metrics-*` search - Clone https://github.com/simianhacker/simian-forge/tree/main - Run ` ./forge --dataset hosts --count 25 --interval 30s` - Check the drop-downs in the metrics grid: https://github.com/user-attachments/assets/77313c81-59fa-41ee-b3f9-60384212cfab - In case of `logs-*` search, for example, the breakdown selector should work as before: <img width="3544" height="1332" alt="image" src="https://github.com/user-attachments/assets/ce33654f-9835-456a-b47f-6a60ed58edd8" /> --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
Closes #233653
Summary
This PR adds an option to clear the selection in the dimension and value selectors. When all dimensions are cleared, it will clear all values as well as those related to the corresponding dimensions. The PR also extracts the toolbar_selector to a shared place and uses it in the metrics grid and the histogram without adding the dependency issue.
How to test
kibana.dev.ymlFROM metrics-*search./forge --dataset hosts --count 25 --interval 30sdimf.mov
logs-*search, for example, the breakdown selector should work as before: