-
Notifications
You must be signed in to change notification settings - Fork 92
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
feat: allow filtering library by publish status #1570
Changes from 4 commits
fd2c357
7cf5d2e
8889017
6d3db8d
73ef429
3f0f95e
2effa32
09d1284
54913f7
777fe25
f28faaa
fd35ce8
419ac2f
dc3f657
daa98b8
19f9420
a338fe8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -192,6 +192,8 @@ const ComponentCard = ({ contentHit }: ComponentCardProps) => { | |
formatted, | ||
tags, | ||
usageKey, | ||
modified, | ||
lastPublished, | ||
} = contentHit; | ||
const componentDescription: string = ( | ||
showOnlyPublished ? formatted.published?.description : formatted.description | ||
|
@@ -216,6 +218,7 @@ const ComponentCard = ({ contentHit }: ComponentCardProps) => { | |
</ActionRow> | ||
)} | ||
openInfoSidebar={() => openComponentInfoSidebar(usageKey)} | ||
hasUnpublishedChanges={modified >= (lastPublished ?? 0)} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: Why not pull this from the added There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ohh nice catch |
||
/> | ||
); | ||
}; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,86 @@ | ||
import React from 'react'; | ||
import { | ||
Badge, | ||
Form, | ||
Menu, | ||
MenuItem, | ||
} from '@openedx/paragon'; | ||
import { FilterList } from '@openedx/paragon/icons'; | ||
import SearchFilterWidget from './SearchFilterWidget'; | ||
import { useSearchContext } from './SearchManager'; | ||
import { PublishStatus } from './data/api'; | ||
|
||
/** | ||
* A button with a dropdown that allows filtering the current search by publish status | ||
*/ | ||
const FilterByPublished: React.FC<Record<never, never>> = () => { | ||
const { | ||
publishStatus, | ||
publishStatusFilter, | ||
setPublishStatusFilter, | ||
} = useSearchContext(); | ||
|
||
const clearFilters = React.useCallback(() => { | ||
setPublishStatusFilter([]); | ||
}, []); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Selecting a Published Status filter doesn't show the "clear filters" link? Need to modify |
||
|
||
const toggleFilterMode = React.useCallback((mode: PublishStatus) => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bug: Selecting one publish_status filter clears all the others? Maybe see There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it was because I forgot to add the setter to the dependencies. this was somehow breaking the tags filter tests so thank you so much for finding this :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thank you for working out a simple fix! it's working great now. |
||
setPublishStatusFilter(oldList => { | ||
if (oldList.includes(mode)) { | ||
return oldList.filter(m => m !== mode); | ||
} | ||
return [...oldList, mode]; | ||
}); | ||
}, []); | ||
|
||
return ( | ||
<SearchFilterWidget | ||
appliedFilters={[]} | ||
label="Publish Status" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please extract "Publish Status" to a message so it can be translated. |
||
clearFilter={clearFilters} | ||
icon={FilterList} | ||
> | ||
<Form.Group className="mb-0"> | ||
<Form.CheckboxSet | ||
name="block-type-filter" | ||
value={publishStatusFilter} | ||
> | ||
<Menu className="block-type-refinement-menu" style={{ boxShadow: 'none' }}> | ||
<MenuItem | ||
as={Form.Checkbox} | ||
value={PublishStatus.Published} | ||
onChange={() => { toggleFilterMode(PublishStatus.Published); }} | ||
> | ||
<div> | ||
Published | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please extract these "Published", "Modified since publish", and "Never published" labels to messages so they can be translated. |
||
{' '}<Badge variant="light" pill>{publishStatus[PublishStatus.Published] ?? 0}</Badge> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think you need to have this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (And strictly speaking, the Badge should be part of the translated text so that RTL languages can put the count badge to the left of the text. But there's a number of places where we'd need to fix this, so it's ok if you don't do it here.) |
||
</div> | ||
</MenuItem> | ||
<MenuItem | ||
as={Form.Checkbox} | ||
value={PublishStatus.Modified} | ||
onChange={() => { toggleFilterMode(PublishStatus.Modified); }} | ||
> | ||
<div> | ||
Modified since publish | ||
{' '}<Badge variant="light" pill>{publishStatus[PublishStatus.Modified] ?? 0}</Badge> | ||
</div> | ||
</MenuItem> | ||
<MenuItem | ||
as={Form.Checkbox} | ||
value={PublishStatus.NeverPublished} | ||
onChange={() => { toggleFilterMode(PublishStatus.NeverPublished); }} | ||
> | ||
<div> | ||
Never published | ||
{' '}<Badge variant="light" pill>{publishStatus[PublishStatus.NeverPublished] ?? 0}</Badge> | ||
</div> | ||
</MenuItem> | ||
</Menu> | ||
</Form.CheckboxSet> | ||
</Form.Group> | ||
</SearchFilterWidget> | ||
); | ||
}; | ||
|
||
export default FilterByPublished; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,7 +10,11 @@ import { MeiliSearch, type Filter } from 'meilisearch'; | |
import { union } from 'lodash'; | ||
|
||
import { | ||
CollectionHit, ContentHit, SearchSortOption, forceArray, | ||
CollectionHit, | ||
ContentHit, | ||
SearchSortOption, | ||
forceArray, | ||
type PublishStatus, | ||
} from './data/api'; | ||
import { useContentSearchConnection, useContentSearchResults } from './data/apiHooks'; | ||
|
||
|
@@ -23,10 +27,13 @@ export interface SearchContextData { | |
setBlockTypesFilter: React.Dispatch<React.SetStateAction<string[]>>; | ||
problemTypesFilter: string[]; | ||
setProblemTypesFilter: React.Dispatch<React.SetStateAction<string[]>>; | ||
publishStatusFilter: PublishStatus[]; | ||
setPublishStatusFilter: React.Dispatch<React.SetStateAction<PublishStatus[]>>; | ||
tagsFilter: string[]; | ||
setTagsFilter: React.Dispatch<React.SetStateAction<string[]>>; | ||
blockTypes: Record<string, number>; | ||
problemTypes: Record<string, number>; | ||
publishStatus: Record<string, number>; | ||
extraFilter?: Filter; | ||
canClearFilters: boolean; | ||
clearFilters: () => void; | ||
|
@@ -99,6 +106,7 @@ export const SearchContextProvider: React.FC<{ | |
const [searchKeywords, setSearchKeywords] = React.useState(''); | ||
const [blockTypesFilter, setBlockTypesFilter] = React.useState<string[]>([]); | ||
const [problemTypesFilter, setProblemTypesFilter] = React.useState<string[]>([]); | ||
const [publishStatusFilter, setPublishStatusFilter] = React.useState<PublishStatus[]>([]); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When you resolve the conflicts for this file, please note that all of the search state variables are now managed as URL parameters, so this status should be too. Should look something like this: const [publishStatusFilter, setPublishStatusFilter] = useStateOrUrlSearchParam<PublishStatus>(
[],
'published',
(value: string) => Object.values(PublishStatus).find((enumValue) => value === enumValue),
(value: PublishStatus) => value.toString(),
skipUrlUpdate,
); References:
|
||
const [tagsFilter, setTagsFilter] = React.useState<string[]>([]); | ||
const [usageKey, setUsageKey] = useStateWithUrlSearchParam( | ||
'', | ||
|
@@ -163,6 +171,7 @@ export const SearchContextProvider: React.FC<{ | |
searchKeywords, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Some UI questions, feel free to bounce these off the UX folks if you agree :)
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @jmakowski1123 @lizc577 @sdaitzman @marcotuts any comments on this? ^ There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @jmakowski1123 @lizc577 @sdaitzman @marcotuts gentle nudge on this since we're blocked waiting for a decision here. |
||
blockTypesFilter, | ||
problemTypesFilter, | ||
publishStatusFilter, | ||
tagsFilter, | ||
sort, | ||
skipBlockTypeFetch, | ||
|
@@ -178,6 +187,8 @@ export const SearchContextProvider: React.FC<{ | |
setBlockTypesFilter, | ||
problemTypesFilter, | ||
setProblemTypesFilter, | ||
publishStatusFilter, | ||
setPublishStatusFilter, | ||
tagsFilter, | ||
setTagsFilter, | ||
extraFilter, | ||
|
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.
Please extract "Unpublished changes" into a message so it can be translated.