fix: Video Gallery filters and sorting #368

xitij2000 · 2023-08-07T14:02:26Z

This PR fixes the filtering and sorting on the video gallery page and also adds support for a checkbox-based filtering mechanism allowing multiple filters to be selected.

Screenshots:

demo-of-video-filter.2023-08-14.14-22.mp4

Testing instructions:

The filtering and sorting should work, and all tests should pass.

openedx-webhooks · 2023-08-07T14:02:36Z

Thanks for the pull request, @xitij2000! Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

supporting documentation
Open edX discussion forum threads
timeline information ("this must be merged by XX date", and why that is)
partner information ("this is a course on edx.org")
any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

codecov · 2023-08-12T03:45:27Z

Codecov Report

Patch coverage: 98.38% and project coverage change: -0.09% ⚠️

Comparison is base (e9c0f6c) 90.35% compared to head (1b3c587) 90.27%.
Report is 1 commits behind head on main.

❗ Current head 1b3c587 differs from pull request most recent head fb7caff. Consider uploading reports for the commit fb7caff to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #368      +/-   ##
==========================================
- Coverage   90.35%   90.27%   -0.09%     
==========================================
  Files         226      227       +1     
  Lines        4022     4030       +8     
  Branches      798      799       +1     
==========================================
+ Hits         3634     3638       +4     
- Misses        369      372       +3     
- Partials       19       20       +1

Files Changed	Coverage Δ
src/editors/containers/VideoGallery/utils.js	`100.00% <ø> (ø)`
src/editors/sharedComponents/BaseModal/index.jsx	`100.00% <ø> (ø)`
...rs/sharedComponents/SelectionModal/GalleryCard.jsx	`100.00% <ø> (ø)`
...ditors/sharedComponents/SelectionModal/messages.js	`100.00% <ø> (ø)`
src/editors/containers/VideoGallery/hooks.js	`83.11% <96.42%> (-10.79%)`	⬇️
src/editors/containers/EditorContainer/hooks.js	`100.00% <100.00%> (ø)`
src/editors/containers/VideoGallery/index.jsx	`100.00% <100.00%> (ø)`
src/editors/containers/VideoUploadEditor/hooks.js	`100.00% <100.00%> (ø)`
...ditors/sharedComponents/SelectionModal/Gallery.jsx	`84.61% <100.00%> (-15.39%)`	⬇️
...nents/SelectionModal/MultiSelectFilterDropdown.jsx	`100.00% <100.00%> (ø)`
... and 2 more

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

farhaanbukhsh · 2023-08-17T06:28:44Z

src/editors/containers/VideoUploadEditor/hooks.js

 export const uploader = () => {
-  const [textInputValue, settextInputValue] = module.state.textInputValue('');
+  const [textInputValue, setTextInputValue] = module.state.textInputValue('');
  return {
    textInputValue,
-    settextInputValue,
+    setTextInputValue,
  };
 };


@xitij2000 you might want to undo this since I fixed it in my PR :)

Sure! I think this entire function can be removed since it just proxies a call to React.useState in the end.

farhaanbukhsh · 2023-08-17T06:30:41Z

src/editors/containers/VideoGallery/hooks.js

+export const useSearchAndSortProps = () => {
+  const [searchString, setSearchString] = React.useState('');
+  const [sortBy, setSortBy] = React.useState(sortKeys.dateNewest);
+  const [filterBy, setFilterBy] = React.useState(filterKeys.videoStatus);
+  const [hideSelectedVideos, setHideSelectedVideos] = React.useState(false);


farhaanbukhsh · 2023-08-17T06:34:31Z

src/editors/containers/VideoGallery/index.jsx

+export const VideoGallery = () => {
+  const rawVideos = useSelector(selectors.app.videos);
+  const isLoaded = useSelector(
+    (state) => selectors.requests.isFinished(state, { requestKey: RequestKeys.fetchVideos }),
+  );
+  const isFetchError = useSelector(
+    (state) => selectors.requests.isFailed(state, { requestKey: RequestKeys.fetchVideos }),
+  );
+  const isUploadError = useSelector(
+    (state) => selectors.requests.isFailed(state, { requestKey: RequestKeys.uploadVideo }),
+  );


Is this pattern better than passing the selectors as a state to props?

The older mechanism of passing them as props makes it harder to understand where the data is coming from since some params are magically filed in and others need to be provided. I find this more explicit.

Ideally, I'd also prefer if we had selectors like selectors.requests.fetchVideos which return a status like 'in_progress', 'done' etc. and then functions called isFinished etc to resolve that. That would make the above calls:

const isLoaded = isFinished(useSelector(selectors.requests.fetchVideos));

That seems a lot cleaner than the current status quo, however it was a bigger refactoring than I had time for.

farhaanbukhsh

👍

Thanks @xitij2000 this an amazing improvement to what we had.

✅ I tested this: running on devstack
✅ I read through the code
❌ I checked for accessibility issues
❌ Includes documentation
❌ I made sure any change in configuration variables is reflected in the corresponding client's configuration-secure repository.

xitij2000 · 2023-08-17T07:46:59Z

src/editors/sharedComponents/SelectionModal/Gallery.jsx

-  if (!show) {
-    return null;
-  }


I removed this because I didn't see a good reason this should be a prop. If a parent component wants to hide this component it can just not render it instead of passing this prop and having it not render itself.

The sort and filter UI of the video gallery was not working, this fixes that issue, and also adds a new UI for filering videos that allows filtering videos to include more than one status. It also fixes the hooks related to VideoGallery to avoid potential bugs in the future and updates tests to use react testing library instead of enzyme. It also reduces the padding in gallery page.

kenclary

ok, looks good enough.

openedx-webhooks · 2023-08-23T13:44:00Z

@xitij2000 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Aug 7, 2023

xitij2000 force-pushed the kshitij/fix-video-sort-filter branch 3 times, most recently from cac1eed to 7ffc39a Compare August 12, 2023 03:39

xitij2000 force-pushed the kshitij/fix-video-sort-filter branch 6 times, most recently from 5d86789 to 1b3c587 Compare August 16, 2023 15:32

farhaanbukhsh self-requested a review August 17, 2023 05:24

farhaanbukhsh reviewed Aug 17, 2023

View reviewed changes

farhaanbukhsh approved these changes Aug 17, 2023

View reviewed changes

xitij2000 commented Aug 17, 2023

View reviewed changes

xitij2000 force-pushed the kshitij/fix-video-sort-filter branch from 1b3c587 to fb7caff Compare August 17, 2023 07:53

kenclary approved these changes Aug 23, 2023

View reviewed changes

kenclary merged commit 45e4bc5 into openedx:main Aug 23, 2023

xitij2000 deleted the kshitij/fix-video-sort-filter branch August 24, 2023 09:55

CefBoud mentioned this pull request Oct 26, 2023

fix: revert to single-value video gallery dropdown filter #414

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Video Gallery filters and sorting #368

fix: Video Gallery filters and sorting #368

xitij2000 commented Aug 7, 2023 •

edited

Loading

openedx-webhooks commented Aug 7, 2023 •

edited

Loading

codecov bot commented Aug 12, 2023 •

edited

Loading

farhaanbukhsh Aug 17, 2023

xitij2000 Aug 17, 2023

farhaanbukhsh Aug 17, 2023

farhaanbukhsh Aug 17, 2023

xitij2000 Aug 17, 2023

farhaanbukhsh left a comment

xitij2000 Aug 17, 2023

kenclary left a comment

openedx-webhooks commented Aug 23, 2023

fix: Video Gallery filters and sorting #368

fix: Video Gallery filters and sorting #368

Conversation

xitij2000 commented Aug 7, 2023 • edited Loading

openedx-webhooks commented Aug 7, 2023 • edited Loading

codecov bot commented Aug 12, 2023 • edited Loading

Codecov Report

farhaanbukhsh Aug 17, 2023

Choose a reason for hiding this comment

xitij2000 Aug 17, 2023

Choose a reason for hiding this comment

farhaanbukhsh Aug 17, 2023

Choose a reason for hiding this comment

farhaanbukhsh Aug 17, 2023

Choose a reason for hiding this comment

xitij2000 Aug 17, 2023

Choose a reason for hiding this comment

farhaanbukhsh left a comment

Choose a reason for hiding this comment

xitij2000 Aug 17, 2023

Choose a reason for hiding this comment

kenclary left a comment

Choose a reason for hiding this comment

openedx-webhooks commented Aug 23, 2023

xitij2000 commented Aug 7, 2023 •

edited

Loading

openedx-webhooks commented Aug 7, 2023 •

edited

Loading

codecov bot commented Aug 12, 2023 •

edited

Loading