Feature Sliced review

[azinit]

Опишу здесь основные моменты, надеюсь поможет 🍰

UPD: Буду офк накидывать чисто со стороны проектирования по FS

PR не оч подходил, т.к. нужно еще отсылаться и на не задетые диффом файлы

[azinit]

Начну с того, что "хорошо"))

praise: Глобальные и инициализирующие стили - по канонам 🚀

loripsum-generator/src/app/index.css

Lines 3 to 4 in 2d9e07e

	@import './styles/variables.css';
	@import './styles/reset.css';

[azinit]

praise: Выделять сразу базовый shared UIKit - тоже топ

loripsum-generator/src/shared/ui/index.jsx

Lines 1 to 7 in 2d9e07e

	export { Button } from './button';
	export { Title } from './title';
	export { Paragraph } from './paragraph';
	export { Loader } from './loader';
	export { Output } from './output';
	export { InputNumber } from './input-number';
	export { Form } from './form';

[azinit]

praise: Тож отлично разделять явно апишки ресурсов, которые используются

loripsum-generator/src/shared/api/index.js

Line 1 in 2d9e07e

export * as mashapeApi from './mashape';

[azinit]

praise: Тоже неплохо обыграл View и ViewModel )

Чтоб компонент не разбухал

loripsum-generator/src/features/copy-to-clipboard/ui.jsx

Lines 3 to 21 in 2d9e07e

	import { useCopyClipboard } from './lib';
	import styled from 'styled-components';
	import copy from './icon/copy.svg';
	import checkmark from './icon/checkmark.svg';
	export const CopyToClipboard = ({ data }) => {
	const [isCopied, setCopied] = useCopyClipboard(data, {
	successDuration: 1000,
	});
	return (
	<StyledCopyToClipboard onClick={setCopied}>
	<img src={isCopied ? checkmark : copy} alt="Copy." />
	</StyledCopyToClipboard>
	);
	};

[azinit]

praise: Отлично, что сразу есть понимание, что надо заводить сущность, даже если там хранится только модель (и даже если используется в одном месте)

loripsum-generator/src/entities/text/model.js

Lines 1 to 7 in 2d9e07e

	import { createStore, createEffect } from 'effector';
	import { mashapeApi } from '../../shared/api/';
	export const getTextContentFx = createEffect((num = 1) => mashapeApi.getTextContent(num));
	export const $textContent = createStore([])
	.on(getTextContentFx.doneData, (_, data) => data);

[azinit]

Теперь к toImprove)

issue(critical): Сейчас "пойдет", но позже роутинг должен явно декларироваться)

suggestion(critical): Даж если роутера нет и страница по факту одна - то желательно этот момент отдельно задекларировать. Чтоб когда появятся доп. страницы - не было больно все это мигрировать (ну и чтобы в app логика не разбухала)

Можно прям на уроне pages/index.tsx реэкспортить обертку, где напрямую home юзается. Но для аппа это будет обычный роутинг, который потом всегда будет легко поменять

loripsum-generator/src/app/index.jsx

Lines 4 to 7 in 2d9e07e

	import { Home } from '../pages/home';
	export const App = () => (
	<Home />

[azinit]

issue(critical): Так у нас App или HomePage уровень абстракции?)

suggestion(critical): Тут лучше бы явно определить разметку и стили app в соответствующем слое, а для страниц оставить их уровень реализации

thoughts:
(не говорю вставлять сразу хедер/футер в апп - но это явно не должно описываться в странице - потом будет больно добавлять новые иначе)

loripsum-generator/src/pages/home/index.css

Lines 1 to 12 in 2d9e07e

	/* App styles */
	.app {
	padding: 80px 10px 0;
	margin: 0 auto;
	max-width: 880px;
	}
	@media (max-width: 550px) {
	.app {
	padding-top: 30px;
	}

[azinit]

issue(critical): Можно заметить и по стилям и по разметке - что там уж слишком разбухшая логика)

thoughts: При этом тот же CopyToClipboard почему-то вынесен в отдельную фичу, но не остальное)

suggestion(critical): Щас не особо больно, но по-хорошему бы декомпозировать это все дело:

• features/generate-text - форма для генерации lorem-заглушки со всеми нужными инпутами
  Пока кажется еще сильней ее дробить не имеет смысл, разве что CopyToClipboard можно действительно отдельно подержать (хотя это возможно вообще слой shared).
  Также можно временно перенести стор связанный с текстом ближе к фиче, а там посмотреть - насколько стоит делать его глобальным (локальный стор всегда предпочтительней глобального, т.к. такое проще дебажить/развивать - хотя это мы уже в датафлоу уходим)
• shared/ui/empty - обычно для этого даж отдельный компонент выделяют (можно глянуть здесь)

thoughts: Вот. И даж при такой декомпозиции - у тебя уже фича берет на себя значительную часть ответственности. А страница по сути лишь композит и дополняет нужной логикой на своем уровне

loripsum-generator/src/pages/home/index.jsx

Lines 30 to 55 in 2d9e07e

	<Form onSubmit={handleSubmit} $marginTop="31px" $grid $gap="12px">
	<InputNumber
	labelText="Paragraphs:"
	value={input}
	onChange={(e) => setInput(e.target.value)}
	min={1}
	max={999}
	$color="var(--color-blue-darker)"
	/>
	<Button
	type="submit"
	$color="#FFFFFF"
	$background="var(--color-blue-medium)"
	$backgroundHover="var(--color-blue-lightest)"
	$backgroundActive="var(--color-blue-darker)"
	>
	Generate
	</Button>
	</Form>
	<Output $padding="37px" $marginTop="24px" $height="550px" $gap="32px" $center>
	<CopyToClipboard data={textContentJoined} />

loripsum-generator/src/pages/home/index.css

Lines 14 to 37 in 2d9e07e

	/* Form */
	.form {
	display: grid;
	grid-auto-flow: column;
	justify-content: center;
	align-items: center;
	grid-gap: 12px;
	margin-top: 31px;
	}
	@media (max-width: 362px) {
	.form {
	grid-auto-flow: row;
	}
	}
	/* Empty */
	.empty {
	color: #617D98;
	text-align: center;

[yesnoruly]

Видимо на этом моменте придётся выносить CopyClipboard в shared, чтобы с чистой душой напрямую использовать его во view.jsx. Как думаешь норм затея или лучше оставить пока как есть и через композицию (я не совсем понял как именно xd)

[azinit]

В целом да, можно было бы и оставить в фичах с композицией
Но тут прям как будто все сигналы на то, что это shared/lib скорее

[azinit]

@yesnoruly Некритично, но я бы всеж copy-clipboard и все обвязки прям в shared/lib/clipboard положил)

А то прям ось изменений как будто искуственно размазывается, хотя могла быть локализована

[yesnoruly]

Fixed

[azinit]

Пушка бомба)

[azinit]

nitpic(non-critical): Некритично, но для консистнетности и конвенциональности - папку с иконками назвал бы assets

loripsum-generator/src/features/copy-to-clipboard/ui.jsx

Lines 7 to 8 in 2d9e07e

	import copy from './icon/copy.svg';
	import checkmark from './icon/checkmark.svg';

thoughts: Ну и да - тож надо следить за сложностью папки, чтоб было считываемо какая логика к какому сегменту относится

Пока норм, но потом по мере сложности фичи - понадобится директории выделять почти наверняка

[azinit]

thoughts: На самом деле большой вопрос как уже говорил - является ли это фичой... Пока можно оставить, но что-то мне подсказывает, что придется в shared/lib положить, т.к. логика достаточно близкая к веб-платформе)

thoughts: Кста да - чтобы это попало в фичу генерации текста - не обязательно напрямую дергать - можно как раз на уроне страницы композить как считаешь нужным
(а фича чтобы просто имела пропс - через который можно передать что угодно)

loripsum-generator/src/features/copy-to-clipboard/ui.jsx

Lines 2 to 20 in 2d9e07e

	import { useCopyClipboard } from './lib';
	import styled from 'styled-components';
	import copy from './icon/copy.svg';
	import checkmark from './icon/checkmark.svg';
	export const CopyToClipboard = ({ data }) => {
	const [isCopied, setCopied] = useCopyClipboard(data, {
	successDuration: 1000,
	});
	return (
	<StyledCopyToClipboard onClick={setCopied}>
	<img src={isCopied ? checkmark : copy} alt="Copy." />
	</StyledCopyToClipboard>
	);
	};

loripsum-generator/src/features/copy-to-clipboard/lib.js

Lines 1 to 10 in 2d9e07e

	import { useState, useEffect } from 'react';
	import copy from 'copy-to-clipboard';
	export const useCopyClipboard = (
	text,
	options,
	) => {
	const [isCopied, setIsCopied] = useState(false);
	const successDuration = options?.successDuration;

[azinit]

nitpick(non-critical): Пока некритично, но прям цепляется глаз за относительные импорты слоев)

Потом это может неплохо так выстрелить в колено, поэтому для импорта внешних ресурсов лучше сразу использовать абсолютные импорты

См. подробнее здесь

loripsum-generator/src/pages/home/index.jsx

Lines 6 to 11 in 2d9e07e

	import { CopyToClipboard } from '../../features/copy-to-clipboard';
	import {
	InputNumber, Button, Title, Output, Form, Loader, Paragraph,
	} from '../../shared/ui';
	import { $textContent, $textContentJoined, getTextContentFx } from '../../entities/text';

[azinit]

suggestion(non-critical): Пока норм, и в целом оправдано с CSSinJS что каждый компонент единым файлом, а не директорией.

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

Select,Button => ComboBox

Можно глянуть атомик и по нему потом раскладывать компоненты, чтобы они друг друга не кросс импортили жестко

[azinit]

За остальное глаз не зацепился вродь, но и у проекта сложность не шибко большая на старте)

На самом деле еще стоит понимать - что концепции эти часто мешает применять уже существующее понимание принципов (GRASP, DRY) + нехватка контекста, чтобы правильно применять концепции FS (критерии слоев)

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

[azinit]

Если все эти правки внести, то вполне можно добавить репе лейбл feature-sliced и можно будет к нам в примеры FS проектов занести спокойно, если есть желание)

[azinit]

Или посмотреть что есть в похожих lorem-генераторах

[yesnoruly]

@azinit Готово, ждёт своего ревью)

[azinit]

@yesnoruly Все топчик, спасибо что практически все правки внес 🚀

[azinit]

Уточнить разве что хотел бы - а тыж вроде хотел добавлять в примеры, когда добавишь доп функционал, не?)
Там темизацию/доп фичи для генератора

Или пока не особо для этого желание/время есть?)

[yesnoruly]

@azinit Да, времени особо сейчас не густо. Но я дорожную карту проекта основную наметил, думал уже в процессе всё дописывать)

[azinit]

Если останутся вопросы - пингуй)

[yesnoruly]

Так-с, ну вроде все критикал и около того моменты пофиксены)

финальный пр смотреть тут (#11)

[azinit]

@yesnoruly Плюс кста некритично, но можно такую блямбу добавить при желании)

И спасибо что лейбл указал

[![FeatureSliced](https://img.shields.io/badge/Powered%20by-%F0%9F%8D%B0%20Feature%20Sliced-%235c9cb5)](https://feature-sliced.design/)

[yesnoruly]

@azinit На счёт этого:

Не надо было этого делать наверное, да?) Ну впринципе если подумать - не настолько мажорный пр, чтобы по списочку тому проходиться)

[azinit]

Ну это конвенции прост)

Не зря же написано:

Даж если тебе фактически не приходилось делать, не обязательно было прям сносить нафиг чеклист)
(хотя по сути ты как минимум пункты про "CONTRIBUTING" и "описание" выполнил)

[yesnoruly]

Понял, буду знать)