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

Add new example - IT bookstore #659

Merged
merged 1 commit into from Apr 3, 2024

Conversation

UmttikhinaDasha
Copy link
Contributor

Пример с использованием React, TypeScript, Redux Toolkit для просмотра и поиска книг, связанных с IT сферой.

https://github.com/UmttikhinaDasha/IT-Bookstore

Copy link

netlify bot commented Mar 28, 2024

👷 Deploy request for pr-fsd pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit de05da9

@illright
Copy link
Contributor

Привет, спасибо за пример! Есть несколько пунктов фидбека:

  1. Немного смущает папка shared/lib/hooks. В целом, FSD рекомендует не группировать вещи по тому, чем они являются (хуки, например), а вместо этого группировать по назначению (например, это для UI, а другое — для API). В случае shared/lib, идеальной для этой папки структурой является плоская коллекция мини-библиотечек, каждая из которых строго определяет свое назначение. В данном случае может быть тяжело выделить useDebounce и usePrevious в отдельные библиотеки, и тем не менее, назначение у них разное. Я бы предложил lib/debounce и lib/state соответственно. category можно, в целом, оставить и так, но это название не очень передает мне, какой код в этот файл можно добавить, а какой — нельзя. Вижу, что эта функция используется только в одном месте, я бы не парился и просто перенес ее туда, к месту использования.

  2. (не блокер) Вайлдкард-экспорты (export * from) немного мешают цели индекс-файлов ограничить публичный API. В будущем может быть сложнее это изменить, поэтому лучше сразу прописывать экспорты вручную. Заодно это будет еще одним аргументом не класть что-то в shared, а оставить по месту использования

  3. (не блокер) Советую выдерживать постоянство в названиях сущностей между единственным и множественным числом. Либо так (authors), либо иначе (book), а то на этой почве могут возникать непродуктивные разногласия между разработчиками в команде

  4. Заметил, что фича toggleTheme используется только в одном месте, советую тоже перенести к месту использования. В целом, чем меньше фич — тем лучше, проще находиться по проекту.

  5. (не блокер) Кажется, в src/widgets/navigationMenu пропущено деление на сегменты. Не большая проблема, но указывает на кое-что другое. Этот виджет тоже используется только в одном месте, думаю, можно безболезненно перенести его в layout сегмент, тогда не придется создавать сегмент :)

  6. Но вот на слое pages деления на сегменты явно не хватает, чтоб в дальнейшем было легче добавлять код туда

Скажи, что думаешь насчет этих пунктов

@UmttikhinaDasha
Copy link
Contributor Author

Спасибо за фидбек!

  1. Согласна, звучит разумно. Папку hooks убрала, перенесла category к месту использования
  2. Поняла, вайлдкард-экспорты убрала
  3. Тут тоже согласна, сущности переименовала и на будущее учту)
  4. toggleTheme перенесла к месту использования
  5. Да, это хорошая идея. Уже перенесла navigationMenu в layout
  6. Сегменты в слое pages добавила

Изменения залила

Copy link
Contributor

@illright illright left a comment

Choose a reason for hiding this comment

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

Супер, спасибо, вливаем

@illright illright merged commit 95aa073 into feature-sliced:master Apr 3, 2024
2 checks passed
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.

None yet

2 participants