search

feature-sliced / documentation

🍰 Architectural methodology for frontend projects
https://feature-sliced.design
MIT License
1.41k stars 146 forks source link

Add cryptolight example #610

Closed Yar56 closed 8 months ago

Yar56 commented 11 months ago

Background

В примере реализуется простое приложение с просмотром криптомонет. Есть пару страниц. Есть фича с добавлением монеты в избранное. Авторизация, регистрация. Есть два api хоста, первый это CoinGecko api, второй свой прокси бэкэнд для firebase (вынес его чтобы фронт приложение не знало про firebase и было удобнее работать)

исходники: https://github.com/Yar56/cryptolight сайт: http://crypto-light.space/

Прошу сделать ревью. Спасибо!)

netlify[bot] commented 11 months ago

Deploy request for pr-fsd accepted.

Name Link
Latest commit e3b04cc6492cfd97c179dc63f3d1c8b1686874e1
Latest deploy log https://app.netlify.com/sites/pr-fsd/deploys/64ccc65bfef3810008ad9d06
Yar56 commented 11 months ago

@azinit fyi

azinit commented 11 months ago

@feature-sliced/core

PUndef commented 10 months ago
PUndef commented 10 months ago
PUndef commented 10 months ago
PUndef commented 10 months ago

смущает немного этот участок кода: const fetchData = useCallback(async () => { return await Promise.all([ coinModel.coinSubModel.getCoinByIdFx({ coinId }), coinModel.coinSubModel.getCoinMarketChartByIdFx({ coinId }) ]); }, []); useEffect(() => { fetchData(); }, []); не пользователь effector'а но насколько знаю один из его плюсов в возможности не завязываться на жизненный цикл компонента react'а, best practice для получения данных вроде в том, чтобы подписаться на маунт страницы это замечание тоже опиональное

PUndef commented 10 months ago

писал выше об этом компоненте: src\shared\lib\navigate\getCoinPageUrl.ts Я бы подумал действительно ли это shared, так как явно привязано к энтити coin и используется только в нём

к тому же почему-то присутствует импорт из pages в shared import { RouteName } from '~/pages/models';

PUndef commented 10 months ago

src\shared\lib\storage\localStorage.ts над этим файлом тоже бы подумал, по названию выглядит, как просто удобный интерфейс для работы с LS по факту же тут явная работа с сущностью User поэтому перенёс бы эту часть в сущность User

PUndef commented 10 months ago

src\widgets\header\Header.tsx import { RouteName } from '~/pages/models'; также импорт с верхнего слоя pages подумай о переносе RouteName в shared/config/routes, чтобы не нарушать правила и нужно разобраться почему линтер тебе не подсвечивает эти места, глазами самому найти такие нарушения по всему проекту достаточно проблематично

PUndef commented 10 months ago

src\widgets\layouts\SuspenseLayout.tsx почему не просто в shared/ui? виджет обычно для композиции нижележащих слоёв тут же только обёртка над лоадером так что подумай о том, почему бы не спустить его в shared

illright commented 10 months ago

@Yar56 что думаешь насчет фидбека? Я не смотрел проект сам, поэтому за апдейтами по проекту не следил, хочу понять, в каком состоянии этот ПР

Yar56 commented 10 months ago

@illright @PUndef привет. большинство пунктов обработаны, особенно те которые касаются fsd

netlify[bot] commented 9 months ago

Deploy Preview for pr-fsd ready!

Name Link
Latest commit 40cdd89bb18de690e143ef47a3398a95df673efe
Latest deploy log https://app.netlify.com/sites/pr-fsd/deploys/652fc957db7a0000088ca223
Deploy Preview https://deploy-preview-610--pr-fsd.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

illright commented 9 months ago

Так, посмотрел проект сам, во многом все супер, мне нравится, есть несколько моментиков, которые хотелось бы поправить:

  1. Внутри слайсов на слоях Pages и Widgets нет деления на сегменты. Да, там по сути только один сегмент, ui, и тем не менее. Хочется, чтоб в индексах были ре-экспорты, а код был в сегментах.
  2. В некоторых сегментах слоя Shared нет индексов, например, ui, lib и config.
  3. На слое Pages есть вне-слайсовый файл, pages/index.tsx, который содержит роутинговую логику. По методологии на слоях не может быть кода вне слайсов, роутинг лучше разместить на слое App.
Yar56 commented 8 months ago

Так, посмотрел проект сам, во многом все супер, мне нравится, есть несколько моментиков, которые хотелось бы поправить:

  1. Внутри слайсов на слоях Pages и Widgets нет деления на сегменты. Да, там по сути только один сегмент, ui, и тем не менее. Хочется, чтоб в индексах были ре-экспорты, а код был в сегментах.
  2. В некоторых сегментах слоя Shared нет индексов, например, ui, lib и config.
  3. На слое Pages есть вне-слайсовый файл, pages/index.tsx, который содержит роутинговую логику. По методологии на слоях не может быть кода вне слайсов, роутинг лучше разместить на слое App.

нечайно закрыл))), обработаю

Yar56 commented 8 months ago

@illright добавил везде реэкспорты и убрал indexсы в слоях.