Search improvements + better loader #751
Open
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
Otherwise the previous limit will be used. E.g. if someone searches for "X" and clicks the "Load more" button three times and the searches for "Y", the limit would be higher then the configuredLimit.
It shows a loading spinner, if more data is loaded for a Data List or Data Column List. I also included "hasMore" in it to trigger a loading for the "Load more" button as well.
dcsaszar
requested changes
Feb 12, 2025
| @@ -28,7 +28,15 @@ export const DataBatchContextProvider = connect( | |||
| const configuredLimit = dataScope.limit() ?? 20 | |||
| const [limit, setLimit] = useState(configuredLimit) | |||
| const [initialLimit, setInitialLimit] = useState(configuredLimit) | |||
| const [search, setSearch] = useState('') | |||
| const [search, setSearchState] = useState('') | |||
There was a problem hiding this comment.
- Unfortunate naming. By not following the useState naming convention we can confuse the reader. Let's please stick to the conventions. How about
[query, setQuery]?
Comment on lines
+33
to
+39
| const setSearch = useCallback( | ||
| (query: string) => { | ||
| setSearchState(query) | ||
| setLimit(configuredLimit) | ||
| }, | ||
| [configuredLimit], | ||
| ) |
There was a problem hiding this comment.
- FYI did you consider resetting the limit as an effect of a query change? Would be more straightforward for a reader, but maybe this causes an issue I'm missing right now.
| import { connect, DataScope } from 'scrivito' | ||
| import { Loading } from '../../Components/Loading' | ||
|
|
||
| export const CombinedLoader = connect( |
There was a problem hiding this comment.
- FYI Naming. A "combined loader" rings no bell even after checking the implementation. Idea: DataBatchLoadingIndicator?
| <CombinedLoader | ||
| dataScope={dataScope} | ||
| hasMore={hasMore} | ||
| key={combinedLoaderKey} |
There was a problem hiding this comment.
- FYI Both instances of the Loader use the
DataBatchContextanduseData(). How about inlining the props logic instead of duplicating it in the parents?
| @@ -2,10 +2,12 @@ import { createContext, useCallback, useState } from 'react' | |||
| import { connect, useData, Obj, Widget, ContentTag } from 'scrivito' | |||
|
|
|||
| export const DataBatchContext = createContext<{ | |||
| combinedLoaderKey: string | |||
There was a problem hiding this comment.
- Naming: The name looks like it is coupled to a specific purpose which makes it harder to understand if read in isolation. How about
batchKey?
| dataScope: DataScope | ||
| hasMore: () => boolean | ||
| }) { | ||
| // use side-effects to trigger loading |
There was a problem hiding this comment.
- FYI This is misleading. Typically the loading is triggered by the components that show the content. What we do here is something like "to subscribe" to what we assume the components load ,so we can conditionally show the spinner. Or am I reading this wrong?
| @@ -43,15 +45,17 @@ export const DataBatchContextProvider = connect( | |||
| setLimit(configuredLimit) | |||
| } | |||
|
|
|||
| const key = [ | |||
| const baseKeys = [ | |||
| 'DataBatchContextProvider', | |||
| content.id(), | |||
| attribute, | |||
| id, | |||
| tag, | |||
| search, | |||
| // limit is intentionally not included in the key. Otherwise the component would show a loading spinner on every "load more" click. | |||
There was a problem hiding this comment.
- FYI maybe by using
dataKeyandbatchKeywe could make the code clear enough to go without the comment.
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.
Motivation: When someone clicks on "Load more" a spinner should be shown until all informations are available (more items in the load and the status of "hasMore").
See effect by artificially slowing down
localStorageDataConnection#index: