feat: tag sections, subsections, and the whole course (TEMP) #30
Conversation
src/course-outline/CourseOutline.jsx
Outdated
| // ToDo: Discuss in the PR | ||
| // We could use this to prefetch the tags count for all units/blocks in the course | ||
| // But to prevent refetching the same data when the component is re-rendered, we should | ||
| // configure the query stale time to a reasonable value. | ||
| // IMHO, we should just let each card to fetch the tags count by itself. This is simpler and | ||
| // is working fine. | ||
| // const contentTagPatternMutation = useContentPatternTagsCount(); | ||
| // | ||
| // useEffect(() => { | ||
| // if (courseId) { | ||
| // contentTagPatternMutation.mutate(`${courseId.replace('course-v1', 'block-v1', 1)}*`); | ||
| // } | ||
| // }, [courseId]); |
There was a problem hiding this comment.
@ChrisChV I think we don't need this. The multiple calls were a problem in the Studio, but I think it works smoothly here. What do you think? CC @bradenmacdonald
PS: This is not fully working right now, but I think I can fix it.
There was a problem hiding this comment.
@rpenido I think we don't need it for now, it's a lot for an MVP. Unless a performance analysis says flatly otherwise 😄
About let each cart to fetch the tag count, I think it would increase the number of calls made to the API, one for each card. Using one or the other approach I don't see much difference, but the increase in the number of calls is what makes me lean toward just making one call for all
There was a problem hiding this comment.
I think it is working smoothly even when making A LOT of API calls.
tag_course_outline.webm
We could tweak it by setting a stale time for each query, but I think we should do this for the whole app instead of on a query-by-query basis.
There was a problem hiding this comment.
Our API to get the counts for every item in the course should be pretty fast. I would lean toward just using that everywhere, so there's only one API request. Fetching hundreds of separate API requests on the outline page may work fine on devstack but I suspect will cause issues on production. Especially if people don't have HTTP/2 working.
The only downside is when you invalidate the tags of a particular item, you also have to invalidate the counts of everything in the whole course and fetch it again, but I think it's fine.
Also to be clear, each component (e.g. each unit in the outline) should use the same API hook to "fetch tag counts for the whole course". React Query will de-duplicate this to only a single request. Then take the result, search for the ID of the current unit, and display that count, if any.
There was a problem hiding this comment.
The only downside is when you invalidate the tags of a particular item, you also have to invalidate the counts of everything in the whole course and fetch it again, but I think it's fine.
It is possible to solve this: A mutation prefetch the data and populate the queries via setQueryData. So we get all counts on first load and then each component have their own query after that.
The problem with this code is that mounting the component (the unit cards when expanding) trigger the refetch for the query. This could be fixed setting a proper staleTime, but it will need more time.
I will leave it for now and use a single useQuery as you both suggested (but we could go back to this mixed approach if someday the single query performance hit us).
Thank you for the feedback @ChrisChV @bradenmacdonald !
There was a problem hiding this comment.
@rpenido I thought about that too, and it's what I wanted to suggest, but I think there's a problem with it. During the pre-fetch, if the individual cards render before the prefetch is complete, they will still each individually make a request to the API. I looked a lot of the React Query docs like https://tanstack.com/query/v4/docs/framework/react/guides/prefetching and https://tkdodo.eu/blog/seeding-the-query-cache but I couldn't figure out a way to tell React Query not to load the per-item query keys while the prefetch is happening, but still to load them later if they get invalidated.
There was a problem hiding this comment.
but I couldn't figure out a way to tell React Query not to load the per-item query keys while the prefetch is happening, but still to load them later if they get invalidated.
We could call the prefetch mutation at the CourseOutline page and render (the page or the course tree) only after we get the mutation response, as we do with all course data (sections, subsections, etc.).
I think it will be possible if we spend some time with it, but I don't think we should do it now. The current overall responsiveness is pretty good. I don't want to add complexity to solve a problem we don't have yet. 🙂
| <Sheet | ||
| position="right" | ||
| show={isManageTagsDrawerOpen} | ||
| onClose={/* istanbul ignore next */ () => closeManageTagsDrawer()} | ||
| > | ||
| <ContentTagsDrawer | ||
| id={cardId} | ||
| onClose={/* istanbul ignore next */ () => closeManageTagsDrawer()} | ||
| /> | ||
| </Sheet> | ||
| </> | ||
| ); |
There was a problem hiding this comment.
I moved the Drawer, the tagCount and the related states and events to the CardHeader.
Let me know what you think!
| const { data: contentTagCount, isSuccess: isContentTagCountSuccess } = useContentTagsCount(cardId); | ||
|
|
There was a problem hiding this comment.
Each item asks for its tag count. As commented above, we could prefetch it (to prevent multiple API calls), but I think we don't need it.
* feat: add checklist page * fix: failing tests * fix: styling bugs * fix: lint errors * feat: add test fro CourseChecklist * fix: lint errors * feat: add ChecklistSection tests * fix: lint error * fix: missing api reply status
This is a temporary fix for a bug that stems from a paragon component, SelectableBox. So we're using our own copy from fronten-lib-content-components.
* feat: added Sidebar with unit info * feat: added unit location * refactor: added legacy behavior * feat: added live variant * refactor: code refactoring * feat: added tests and translations * feat: added new font size * refactor: after review
* feat: [AXIMST-24] sidebar buttons functional * refactor: removed modal extra className * refactor: refactoring after review
…changes (openedx#135) * feat: [AXIMST-25] added alert notification about unpublished changes * feat: added tests * feat: added translations
* feat: TagCount component * feat: Update ContentTagsDrawer to use it in the MFE * feat: Manage tags menu added on units * feat: Tag count added on unit * feat: Add button feat to Tag count * test: Course Outline api tests * test: Ignore lines that can not be tested * style: Comment added on ContentTagsDrawer * style: Nits on CardHeader
rpenido
force-pushed
the
rpenido/fal-3678-tag-sections-subsections-and-the-whole-course
branch
from
c7ef3a8
to
5c805cb
Compare
rpenido
force-pushed
the
rpenido/fal-3678-tag-sections-subsections-and-the-whole-course
branch
from
6944de6
to
2b09ef1
Compare
No description provided.