Skip to content
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

Search improvements + better loader #751

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 20 additions & 6 deletions src/Components/DataBatchContext.tsx
Original file line number Diff line number Diff line change
@@ -1,11 +1,13 @@
import { createContext, useState } from 'react'
import { createContext, useCallback, useState } from 'react'
import { connect, useData, Obj, Widget, ContentTag } from 'scrivito'

export const DataBatchContext = createContext<{
combinedLoaderKey: string
Copy link
Contributor

@dcsaszar dcsaszar Feb 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • 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?

hasMore: () => boolean
loadMore: () => void
setSearch?: (query: string) => void
}>({
combinedLoaderKey: '',
hasMore: () => true,
loadMore: () => {
throw new Error('loadMore is not provided!')
Expand All @@ -28,22 +30,32 @@ 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('')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • 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]?


const setSearch = useCallback(
(query: string) => {
setSearchState(query)
setLimit(configuredLimit)
},
[configuredLimit],
)
Comment on lines +35 to +41
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • 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.


if (initialLimit !== configuredLimit) {
setInitialLimit(configuredLimit)
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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • FYI maybe by using dataKey and batchKey we could make the code clear enough to go without the comment.

].join('-')
]

const combinedLoaderKey = [...baseKeys, limit].join('-')

const transform = { limit, search }

Expand All @@ -61,11 +73,13 @@ export const DataBatchContextProvider = connect(
const loadMore = () => setLimit((prevLimit) => prevLimit + configuredLimit)

return (
<DataBatchContext.Provider value={{ hasMore, loadMore, setSearch }}>
<DataBatchContext.Provider
value={{ combinedLoaderKey, hasMore, loadMore, setSearch }}
>
<ContentTag
tag={tag}
id={id}
key={key}
key={baseKeys.join('-')}
content={content}
attribute={attribute}
dataContext={dataScope.transform(transform)}
Expand Down
33 changes: 22 additions & 11 deletions src/Widgets/DataColumnListWidget/DataColumnListWidgetComponent.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,15 @@ import { DataColumnListWidget } from './DataColumnListWidgetClass'
import { EditorNote } from '../../Components/EditorNote'
import { Loading } from '../../Components/Loading'
import { DataErrorEditorNote } from '../../Components/DataErrorEditorNote'
import { useContext } from 'react'
import { DataBatchContext } from '../../Components/DataBatchContext'
import { CombinedLoader } from '../DataWidget/CombinedLoader'

provideComponent(
DataColumnListWidget,
({ widget }) => {
const dataScope = useData()
const { combinedLoaderKey, hasMore } = useContext(DataBatchContext)
let dataError: unknown

try {
Expand All @@ -31,17 +35,24 @@ provideComponent(
const columnsCount = widget.get('columnsCount') || '2'

return (
<div className={`row row-cols-1 row-cols-md-${columnsCount}`}>
{dataItems.map((dataItem) => (
<ContentTag
content={widget}
attribute="content"
className="col"
dataContext={dataItem}
key={dataItem.id()}
/>
))}
</div>
<>
<div className={`row row-cols-1 row-cols-md-${columnsCount}`}>
{dataItems.map((dataItem) => (
<ContentTag
content={widget}
attribute="content"
className="col"
dataContext={dataItem}
key={dataItem.id()}
/>
))}
</div>
<CombinedLoader
dataScope={dataScope}
hasMore={hasMore}
key={combinedLoaderKey}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • FYI Both instances of the Loader use the DataBatchContext and useData(). How about inlining the props logic instead of duplicating it in the parents?

/>
</>
)
},
{ loading: Loading },
Expand Down
19 changes: 19 additions & 0 deletions src/Widgets/DataWidget/CombinedLoader.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
import { connect, DataScope } from 'scrivito'
import { Loading } from '../../Components/Loading'

export const CombinedLoader = connect(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  •  FYI Naming. A "combined loader" rings no bell even after checking the implementation. Idea: DataBatchLoadingIndicator?

function CombinedLoader({
dataScope,
hasMore,
}: {
dataScope: DataScope
hasMore: () => boolean
}) {
// use side-effects to trigger loading
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  •  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?

dataScope.take()
hasMore()

return null
},
{ loading: Loading },
)
9 changes: 9 additions & 0 deletions src/Widgets/DataWidget/DataWidgetComponent.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,15 @@ import { DataWidget } from './DataWidgetClass'
import { EditorNote } from '../../Components/EditorNote'
import { Loading } from '../../Components/Loading'
import { DataErrorEditorNote } from '../../Components/DataErrorEditorNote'
import { DataBatchContext } from '../../Components/DataBatchContext'
import { useContext } from 'react'
import { CombinedLoader } from './CombinedLoader'

provideComponent(
DataWidget,
({ widget }) => {
const dataScope = useData()
const { combinedLoaderKey, hasMore } = useContext(DataBatchContext)
let dataError: unknown

try {
Expand Down Expand Up @@ -37,6 +41,11 @@ provideComponent(
key={dataItem.id()}
/>
))}
<CombinedLoader
dataScope={dataScope}
hasMore={hasMore}
key={combinedLoaderKey}
/>
</>
)
},
Expand Down