-
Notifications
You must be signed in to change notification settings - Fork 293
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
fix(storage-browser): clear search on refresh and navigation #6063
fix(storage-browser): clear search on refresh and navigation #6063
Conversation
|
.../react-storage/src/components/StorageBrowser/views/LocationDetailView/LocationDetailView.tsx
Outdated
Show resolved
Hide resolved
1a80d38
export interface SearchProps { | ||
onSearch?: (term: string, includeSubfolders: boolean) => void; | ||
onSearch?: () => void; | ||
onSearchClear?: () => void; | ||
searchQuery?: string; | ||
onSearchQueryChange?: (query: string) => void; | ||
searchPlaceholder?: string; | ||
showIncludeSubfolders?: boolean; | ||
children?: React.ReactNode; | ||
} |
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.
Since you are touching the structure of the component code could you make some updates, can be done in a follow up:
export interface SearchFieldProps {
id?: string;
label?: React.ReactNode
query?: string;
onQuery?: (query: string) => void;
onSearch?: () => void;
onClear?: () => void;
placeholder?: string;
}
@@ -68,23 +65,11 @@ export const Search = ({ | |||
</Field> | |||
<ButtonElement | |||
className={`${BLOCK_NAME}__submit-button`} | |||
onClick={() => onSearch?.(term, subfoldersIncluded)} | |||
onClick={() => onSearch?.()} |
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.
Should be able to just pass onSearch
idiomatically, same with onSearchClear
?
</LabelElement> | ||
</SpanElement> | ||
) : null} | ||
{children ?? null} |
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.
Since we have decoupled the subfolder checkbox we should remove children
and let the parent view wrap the controls for positioning
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.
I know we're trying to get rid of the wrapping divs, I can remove those and the children
from Search at the same time in a follow up, if that makes sense. It's non-trivial to address it here
Description of changes
Issue #, if available
Description of how you validated changes
Checklist
yarn test
passes and tests are updated/addeddocs
,e2e
,examples
, or other private packages.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.