Submit date filters on enter #35491
Merged
Submit date filters on enter #35491
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ranquild
commented
Nov 9, 2023
| await setOperator("Less than"); | ||
| const input = screen.getByPlaceholderText("Enter a number"); | ||
| userEvent.type(input, "{enter}"); | ||
| expect(onChange).not.toHaveBeenCalled(); |
There was a problem hiding this comment.
Here we test that the validation logic works - it's not possible to submit an invalid filter.
There was a problem hiding this comment.
I'm a bit confused - I understood that the issue is both about filter pills & filter step in GUI editor (see others also testing pills 1, 2) but no components from frontend/src/metabase/query_builder/components/filters/pickers are covered here.
I also don't understand why we have separate versions of these inputs for the GUI editor and for filter pills - are we in the process of replacing query_builder/components/filters/pickers with common/components?
edit:
Fixes #6552
I understand that #34962 from Milestone 2 from #34112 would have to be completed first.
But I would not say #6552 is fixed until that happens.
I think we should try to reproduce it once again afterwards (I can already foresee some edge cases, like when you're editing filter pills, but mouse cursor is on top of table visualization and you hit enter).
|
@kamilmielnik you're correct that we need to merge filter pills from the second miltestone to fully fix the issue. My plan was to close the issue with 0.49 milestone like we do for other filter-related issues. Given that, could you take a look if the PR solves the original issue at least for the notebook editor? |
kamilmielnik
requested changes
Nov 10, 2023
| @@ -77,17 +78,29 @@ export function StringFilterPicker({ | |||
| ); | |||
| }; | |||
|
|
|||
| const handleSubmit = (event: FormEvent) => { | |||
There was a problem hiding this comment.
With string "is" filter, user is able to choose a value that does not exist yet by typing it, hitting enter, and then ticking a checkbox.
There is an issue if you try to add a second value like that later on - hitting enter will close the popover instead adding a new value to the list.
2023-11-10.19-49-46.mp4
There was a problem hiding this comment.
Removed changes from all filters but dates
| @@ -74,15 +75,25 @@ export function TimeFilterPicker({ | |||
| ); | |||
| }; | |||
|
|
|||
| const handleSubmit = (event: FormEvent) => { | |||
There was a problem hiding this comment.
I cannot choose "Starting from..." with a mouse - the popover closes.
2023-11-10.19-56-57.mp4
There was a problem hiding this comment.
Great catch! Fixed
| @@ -77,17 +78,29 @@ export function StringFilterPicker({ | |||
| ); | |||
| }; | |||
|
|
|||
| const handleSubmit = (event: FormEvent) => { | |||
| event.preventDefault(); | |||
There was a problem hiding this comment.
I can't submit the form with Enter when excluding extraction units
2023-11-10.20-00-42.mp4
There was a problem hiding this comment.
This results in counter-intuitive behavior that's why it's not allowed. When hitting Enter with a focused checkbox, it would toggle its value and submit the form at the same time.
| @@ -67,27 +68,35 @@ export function NumberFilterPicker({ | |||
| ); | |||
| }; | |||
|
|
|||
| const handleSubmit = (event: FormEvent) => { | |||
There was a problem hiding this comment.
Something is wrong here.
- In
masterthe picker looks very differently, here it's a list (perhaps it's a problem in the target branch) - existing options are being added to the list when hitting enter
- existing options don't get selected when hitting enter (should they)?
- the same problem as with the
StringFilterPicker(see my other comment)
2023-11-10.20-04-06.mp4
There was a problem hiding this comment.
Removed changes to all filters but dates from the PR.
There was a problem hiding this comment.
@kamilmielnik btw I verified and in master these issues exist, and I believe the only thing that's different is that the popover gets closed on enter without saving the changes
…ilter-submit-enter # Conflicts: # frontend/src/metabase/common/components/FilterPicker/BooleanFilterPicker/BooleanFilterPicker.tsx # frontend/src/metabase/common/components/FilterPicker/CoordinateFilterPicker/CoordinateColumnSelect.tsx # frontend/src/metabase/common/components/FilterPicker/CoordinateFilterPicker/CoordinateFilterPicker.tsx # frontend/src/metabase/common/components/FilterPicker/CoordinateFilterPicker/CoordinateFilterPicker.unit.spec.tsx # frontend/src/metabase/common/components/FilterPicker/FilterFooter/FilterFooter.tsx # frontend/src/metabase/common/components/FilterPicker/NumberFilterPicker/NumberFilterPicker.tsx # frontend/src/metabase/common/components/FilterPicker/NumberFilterPicker/NumberFilterPicker.unit.spec.tsx # frontend/src/metabase/common/components/FilterPicker/StringFilterPicker/StringFilterPicker.tsx # frontend/src/metabase/common/components/FilterPicker/TimeFilterPicker/TimeFilterPicker.tsx # frontend/src/metabase/common/components/FilterPicker/TimeFilterPicker/TimeFilterPicker.unit.spec.tsx
Current Cypress Test Results Summary✅ 2026 Passing - ❌ 20 Failing - Run may still be in progress, this comment will be updated as current testing workflow or job completes... (Last updated on 11/14/2023 11:19:35am UTC) Run DetailsRunning Workflow E2E Tests on Github Actions Commit: d90094c Started: 11/14/2023 10:56:58am UTC ❌ Failures📄 e2e/test/scenarios/filters/filter-types.cy.spec.js • 11 FailuresTop 1 Common Error Messages
Test Case Results
📄 e2e/test/scenarios/custom-column/cc-data-type.cy.spec.js • 3 FailuresTop 1 Common Error Messages
Test Case Results
📄 e2e/test/scenarios/filters/reproductions/25378-relative-date-on-breakout.cy.spec.js • 1 FailureTest Case Results
📄 e2e/test/scenarios/filters/reproductions/20683-postgres-current-quarter.cy.spec.js • 1 FailureTest Case Results
📄 e2e/test/scenarios/filters/operators.cy.spec.js • 1 FailureTest Case Results
📄 e2e/test/scenarios/filters/filter.cy.spec.js • 1 FailureTest Case Results
📄 e2e/test/scenarios/onboarding/search/search.cy.spec.js • 1 FailureTest Case Results
📄 e2e/test/scenarios/custom-column/custom-column.cy.spec.js • 1 FailureTest Case Results
|
| Test Case | Last 7 days Failures | Last 7 days Flakes |
|---|---|---|
|
scenarios > search applying search filters last_edited_at filter should filter results by Today (last_edited_at=thisday)
Retry 2 • Retry 1 • Initial Attempt |
1.93% (8)8 / 415 runsfailed over last 7 days |
8.19% (34)34 / 415 runsflaked over last 7 days |
📄 e2e/test/scenarios/sharing/reproductions.cy.spec.js • 1 Flake
Test Case Results
| Test Case | Last 7 days Failures | Last 7 days Flakes |
|---|---|---|
|
issue 17657 frontend should gracefully handle the case of a subscription without a recipient (#17657)
Retry 1 • Initial Attempt |
0.24% (1)1 / 412 runfailed over last 7 days |
1.21% (5)5 / 412 runsflaked over last 7 days |
📄 e2e/test/scenarios/actions/actions-on-dashboards.cy.spec.js • 1 Flake
Test Case Results
| Test Case | Last 7 days Failures | Last 7 days Flakes |
|---|---|---|
|
Write Actions on Dashboards (mysql) Actions Data Types properly loads and updates date and time fields for implicit update actions
Retry 1 • Initial Attempt |
0.94% (4)4 / 424 runsfailed over last 7 days |
6.60% (28)28 / 424 runsflaked over last 7 days |
ranquild
commented
Nov 13, 2023
| @@ -76,11 +84,6 @@ function getExcludeMonthOption(month: number): ExcludeValueOption { | |||
| return { value: month, label: date.format("MMMM") }; | |||
| } | |||
|
|
|||
| function getExcludeQuarterOption(quarter: number): ExcludeValueOption { | |||
There was a problem hiding this comment.
dayjs doesn't support ordinal quarters even with advancedFormat plugin
ranquild
commented
Nov 13, 2023
| }; | ||
|
|
||
| const handleIncludeCurrentClick = () => { | ||
| onChange(setIncludeCurrent(value, !includeCurrent)); | ||
| setIsMenuOpened(false); |
There was a problem hiding this comment.
We need to close the menu early to prevent it from closing the outer popover
|
@kamilmielnik I've excluded changes to all filters but dates in this PR. I also fixed an issue with the relative dates filter. I decided not to add "submit" logic to exclude date filters because otherwise the checkbox gets toggled and the form gets submitted at the same and it's not intuitive. Please take a look one more time. Changes to other filters and |
…ilter-submit-enter # Conflicts: # frontend/src/metabase/common/components/DatePicker/ExcludeDatePicker/utils.ts
| > | ||
| {t`Starting from…`} | ||
| </Menu.Item> | ||
| )} | ||
| <Menu.Item | ||
| icon={<Icon name={includeCurrent ? "check" : "calendar"} />} | ||
| onClick={handleIncludeCurrentClick} | ||
| onMouseDown={handleIncludeCurrentClick} // we need to close the menu on mousedown to prevent the outer popover from closing |
There was a problem hiding this comment.
I don't think this is an acceptable solution - the UX is weird.
This fixes it nicely:
diff --git a/frontend/src/metabase/ui/components/overlays/Menu/Menu.styled.tsx b/frontend/src/metabase/ui/components/overlays/Menu/Menu.styled.tsx
index 2e3693cf2c..853a44cc83 100644
--- a/frontend/src/metabase/ui/components/overlays/Menu/Menu.styled.tsx
+++ b/frontend/src/metabase/ui/components/overlays/Menu/Menu.styled.tsx
@@ -6,10 +6,11 @@ export const getMenuOverrides = (): MantineThemeOverride["components"] => ({
defaultProps: {
radius: "sm",
shadow: "md",
- withinPortal: true,
+ withinPortal: false,
},
styles: theme => ({
dropdown: {
+ position: "fixed",
padding: "0.75rem !important",
minWidth: "11.5rem",
},plus, of course:
diff --git a/frontend/src/metabase/common/components/DatePicker/RelativeDatePicker/DateIntervalPicker/DateIntervalPicker.tsx b/frontend/src/metabase/common/components/DatePicker/RelativeDatePicker/DateIntervalPicker/DateIntervalPicker.tsx
index 8e687e9981..7701e00e36 100644
--- a/frontend/src/metabase/common/components/DatePicker/RelativeDatePicker/DateIntervalPicker/DateIntervalPicker.tsx
+++ b/frontend/src/metabase/common/components/DatePicker/RelativeDatePicker/DateIntervalPicker/DateIntervalPicker.tsx
@@ -42,7 +42,6 @@ export function DateIntervalPicker({
onChange,
onSubmit,
}: DateIntervalPickerProps) {
- const [isMenuOpened, setIsMenuOpened] = useState(false);
const interval = getInterval(value);
const unitOptions = getUnitOptions(value);
const includeCurrent = getIncludeCurrent(value);
@@ -63,12 +62,10 @@ export function DateIntervalPicker({
const handleStartingFromClick = () => {
onChange(setDefaultOffset(value));
- setIsMenuOpened(false);
};
const handleIncludeCurrentClick = () => {
onChange(setIncludeCurrent(value, !includeCurrent));
- setIsMenuOpened(false);
};
const handleSubmit = (event: FormEvent) => {
@@ -92,7 +89,7 @@ export function DateIntervalPicker({
ml="md"
onChange={handleUnitChange}
/>
- <Menu opened={isMenuOpened} onChange={setIsMenuOpened}>
+ <Menu>
<Menu.Target>
<Button
c="text.2"
@@ -106,7 +103,6 @@ export function DateIntervalPicker({
<Menu.Item
icon={<Icon name="arrow_left_to_line" />}
onClick={handleStartingFromClick}
- onMouseDown={handleStartingFromClick}
>
{t`Starting from…`}
</Menu.Item>
@@ -114,7 +110,6 @@ export function DateIntervalPicker({
<Menu.Item
icon={<Icon name={includeCurrent ? "check" : "calendar"} />}
onClick={handleIncludeCurrentClick}
- onMouseDown={handleIncludeCurrentClick} // we need to close the menu on mousedown to prevent the outer popover from closing
aria-selected={includeCurrent}
data-testid="include-current-interval-option"
>There was a problem hiding this comment.
Related: mantinedev/mantine#4952 (comment)
There was a problem hiding this comment.
Great fix, applied
|
@kamilmielnik I believe I've addressed all the issues. Please let me know if there are others and I fix them in follow up PRs. |
|
Codenotify: Notifying subscribers in CODENOTIFY files for diff cbcbfca...bd64b4f.
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Partially fixes #6552
Allows to submit every date filter with an input via keyboard, i.e.
Enterkey. Exclude filters aren't changed becauseEnteralso modifies the checkbox value and it becomes counter-intuitive.How to verify: