Add Saved Queries #7354
Add Saved Queries #7354
Conversation
|
Everything seems all right! 👍 |
| @@ -1,5 +1,4 @@ | |||
| import * as React from 'react'; | |||
| import { styled } from '@mui/material/styles'; | |||
There was a problem hiding this comment.
when testing the Simple Example, the filterbutton remains "active" after adding a filter from the list. This doesn't happen in next
There was a problem hiding this comment.
Because it remains focused which is actually the correct behavior regarding accessibility
There was a problem hiding this comment.
I disagree. The first added filter should receive the focus. If you don't know which element to focus, I suggest to keep the behavior of the previous FilterButton.
There was a problem hiding this comment.
The thing is, I haven't done anything to make it behave that way so I don't know what to revert
packages/ra-ui-materialui/src/list/filter/RemoveSavedQueryDialog.tsx
Outdated
Show resolved
Hide resolved
|
You still haven't fixed the focus p^roblem on the FilterButton after saving a query |
Because I haven't find the fix yet |
| @@ -214,32 +214,31 @@ You can place these `<FilterList>` anywhere inside a `<List>`. The most common c | |||
|
|
|||
| ```jsx | |||
| import * as React from 'react'; | |||
| import { Card as MuiCard, CardContent, withStyles } from '@mui/material'; | |||
| import { Box, Card, CardContent, styled } from '@mui/material'; | |||
There was a problem hiding this comment.
styled isn't used in this example
docs/useStore.md
Outdated
| @@ -66,3 +66,18 @@ const ChangeDensity = () => { | |||
| ); | |||
| }; | |||
| ``` | |||
|
|
|||
| The validate function can be used to ensure forward compatibility. It must returns a boolean indicating wether the value is valid or not. It will be called in the following cases: | |||
There was a problem hiding this comment.
| The validate function can be used to ensure forward compatibility. It must returns a boolean indicating wether the value is valid or not. It will be called in the following cases: | |
| The validate function can be used to ensure forward compatibility. It must return a boolean indicating wether the value is valid or not. React-admin calls it in the following cases: |
| jest.spyOn(console, 'warn').mockImplementation(() => {}); | ||
| const validate = jest | ||
| .fn() | ||
| .mockReturnValueOnce(true) |
There was a problem hiding this comment.
I understand these mocked values correspond to "moments" of validation. I think this deserves a few comments in this test (and the following).
|
|
||
| const set = useEventCallback( | ||
| (valueParam: T, runtimeDefaultValue: T) => { | ||
| ( |
There was a problem hiding this comment.
I think it doesn't make sense to validate the value passed to the setter. We want to detect when a value was set by a different version than the current code. The setter can only be called using the current version of the code.
| const [value, setValue] = useState(() => getItem(key, defaultValue)); | ||
| const setValueCalled = useRef(false); | ||
|
|
||
| // Validate the value stored in the store |
There was a problem hiding this comment.
I don't understand when this triggers - or rather, I'm afraid it may trigger when a validation has already been called otherwise.
| useEffect(() => { | ||
| if ( | ||
| // Only if it wasn't changed by the setter yet | ||
| !setValueCalled.current && |
There was a problem hiding this comment.
Not robust, imagine this scenario:
- some code calls set in this tab => the value changes, setValueCalled is true, the hook doesn't trigger
- some code calls set in another tab => the value changes, setValueCalled is still true from the past call, the hook doesn't trigger
| @@ -87,7 +133,66 @@ export const FilterButton = (props: FilterButtonProps): JSX.Element => { | |||
| autoFocus={index === 0} | |||
| /> | |||
| ))} | |||
| {savedQueries.map((savedQuery, index) => | |||
There was a problem hiding this comment.
you need to be more defensive here, too
| <HelpIcon /> | ||
| </Tooltip> | ||
| )} | ||
| {savedQueries.map((savedQuery, index) => ( |
No description provided.