[front] feat: Add a tab for "good short videos" #1825
Conversation
There was a problem hiding this comment.
Hello @sigmike
It works well 👌
I made few suggestions, mainly to improve how the user's preferences are retrieved.
| { | ||
| name: 'good-short-videos', | ||
| label: t('entitySelector.goodShortVideos'), | ||
| fetch: async () => { | ||
| return await getGoodShortVideos(); | ||
| }, | ||
| disabled: !isLoggedIn, | ||
| }, |
There was a problem hiding this comment.
(a) I think we can reduce even more the name of the tab (just a personal opinion).
Instead of good short videos, we can display only "short videos" (or even "short vid." and "vid. courtes"). The main goal of this tab is to display short videos, and as long as we don't have a "bad short videos" I think we can safely keep the "good" aspect implicit.
(b) There is a description for each tab, defined in the component TabInfo of the file EntityTabsBox. This description is displayed to new users (users having a low number of comparisons).
We should also add a description to this tab, it could be something like "A random sample of short videos recommended by the community."
| }; | ||
|
|
||
| export const getGoodShortVideos = async (): Promise<Recommendation[]> => { | ||
| const languages = initRecommendationsLanguages(); |
There was a problem hiding this comment.
I think this is the old way to retrieve the languages.
It's useful in the preferences form to initialize the languages when the user hasn't explicitly configured anything, but for logged users it shouldn't be used alone elsewhere in the application. It should be a fallback used when the preferred languages are undefined.
Even if it currently works, I'd suggest to use the primary source of truth first, and fallback to the local storage in case of need.
We can use something like this (I didn't test):
// this code goes in the component VideoInput
const userSettings = useSelector(selectSettings).settings;
const preferredLanguages =
userSettings?.[pollName as PollUserSettingsKeys]
?.recommendations__default_languages;
// preferredLanguages can be passed as a parameter to the function getGoodShortVideos
let languages = preferredLanguages?.join(',') ?? null;
if (languages == null) {
languages = initRecommendationsLanguages();
}There was a problem hiding this comment.
Done in 487c1b3.
I wonder if usePreferredLanguages should get the poll name itself. I think it's good to make it explicit that it depends on the poll, but adding a parameter for that is probably not the best solution. Maybe it should be in the name instead (usePreferredLanguagesFromCurrentPoll for example).
But it brings up another comment: useCurrentPoll is widely used but it does some calculations that are a bit wasteful to repeat. Its return value could be memoized with just the poll as a dependency, but we need to be sure that this value is not muted. Or we could have a useCurrentPollName which doesn't do any calculations, since useCurrentPoll is often used just for the name (25 times out of the 78 uses of useCurrentPoll).
All this is very minor and can be done later though.
There was a problem hiding this comment.
Thank you very much.
About usePreferredLanguages vs usePreferredLanguagesFromCurrentPoll, I don't have a strong preference. I think we can keep usePreferredLanguages for now, and refactor it if we optimize useCurrentPoll.
Regarding useCurrentPoll you are right, it could be good in theory to optimize it, but I'm not sure I understood correctly the risk of poll being muted, and the potential complexity added because of this. Can we talk about it on Discord or during our next meeting?
Related issue #1786
Description
I added the new tab for "good short videos".
I increased the width of the dialog to make all the tabs visible without horizontal scrolling.
Checklist