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

feat: task6 (Redux) #5

Open
wants to merge 1 commit into
base: task5
Choose a base branch
from
Open

feat: task6 (Redux) #5

wants to merge 1 commit into from

Conversation

ViktorGuschin
Copy link
Owner

Evaluation criteria*

3 - All data fetches are implemented as actions and received from store by components
4 - Sorting by release date and rating is implemented as redux actions
5 - Filtering by genre is implemented as redux actions

*Each mark includes previous mark criteria

Copy link

@Tanya-atatakai Tanya-atatakai left a comment

Choose a reason for hiding this comment

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

все очень удобно написано 👍

const [filter, setFilter] = useState('all');
const dispatch = useAppDispatch();

const navItems: string[] = useMemo(() => ['all', 'documentary', 'comedy', 'horror', 'crime'], []);

Choose a reason for hiding this comment

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

если этот набор параметров не будет меняться, то можно его не держать в реактовом компоненте а вынести например за пределы (между 5 и 7 строкой) и будет просто const NAV_ITMES = ['all', 'documentary', 'comedy', 'horror', 'crime']. тогда и useMemo не нужен и в зависимостях эффекта можно не держать

Copy link

Choose a reason for hiding this comment

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

еще для простых конструкций можно JSON.stringify(navItems)
вот тут подробно написано facebook/react#14476 (comment)

if (body) {
config.body = JSON.stringify(body);
}
config.method = body ? 'POST' : 'GET';

Choose a reason for hiding this comment

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

отлично описал function client, единственное может быть потом сложно добавлять методы удаления\обновления которые вроде в сваггере апишки на PUT и DELETE сделаны. еще если не пробовал можешь попробовать axios, облегчит жизнь, не нужно будет конвертировать респонс, параметры запроса и body удобно передавать, есть возможность настроить ретраи и тд


useEffect(() => {
if (searchText?.length > 2) {
dispatch(searchMoviesByTitle(searchText));

Choose a reason for hiding this comment

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

в учебке не так важно, но в проде лучше использовать debounce если поиск происходит на сервере, чтобы на каждый введенный символ (а пользователи обычно вводят быстро) не происходил ненужный запрос

Copy link

@artabr artabr left a comment

Choose a reason for hiding this comment

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

Все супер! Спасибо за практический пример использования extraReducers и createAsyncThunk


const API_URL = 'http://localhost:4000';

export default async function client(endpoint, { body, ...customConfig }: ClientArgs = defaultConfig) {
Copy link

Choose a reason for hiding this comment

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

На мой взгляд тут жестковато написано - по сути это класс, но с первого взгляда очень сложно для восприятия. Но очень изобретательно =)

const [filter, setFilter] = useState('all');
const dispatch = useAppDispatch();

const navItems: string[] = useMemo(() => ['all', 'documentary', 'comedy', 'horror', 'crime'], []);
Copy link

Choose a reason for hiding this comment

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

еще для простых конструкций можно JSON.stringify(navItems)
вот тут подробно написано facebook/react#14476 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants