Skip to content
This repository has been archived by the owner on Jul 28, 2024. It is now read-only.

[FRNT-366] fix surface #64

Merged
merged 1 commit into from
Apr 7, 2021
Merged

[FRNT-366] fix surface #64

merged 1 commit into from
Apr 7, 2021

Conversation

tatinacher
Copy link
Member

@tatinacher tatinacher added documentation Improvements or additions to documentation @atoms Changes inside atoms labels Apr 1, 2021
@github-actions github-actions bot added the lib label Apr 1, 2021
@@ -152,6 +152,26 @@ export const Playground: React.FC<{
);
};

type StateType = {
change: (value: any) => any;
children: (value: any, change: any) => void;
Copy link
Contributor

Choose a reason for hiding this comment

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

это точно children? может лучше как-то иначе назвать пропс , не используя зарезервированные слова?

Copy link
Member

Choose a reason for hiding this comment

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

@Irinaristova точно, это нужно для того, чтобы прокидывать туда функцию.

<State>
  {(value, onChange) => <div />}
</State>

@@ -152,6 +152,26 @@ export const Playground: React.FC<{
);
};

type StateType = {
change: (value: any) => any;
Copy link
Contributor

Choose a reason for hiding this comment

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

а разве тут нельзя задать типы? например ReactNode?

Copy link
Member

Choose a reason for hiding this comment

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

Думаю, это особо смысла не имеет.
Typescript не может проверить типы в mdx, они там вообще не подсвечиваются, можно забить

src/ui/atoms/surface/usage.mdx Outdated Show resolved Hide resolved
Comment on lines 162 to 166
const [value, setValue] = React.useState(false);

React.useEffect(() => {
setValue(initial);
}, [initial]);
Copy link
Member

Choose a reason for hiding this comment

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

Не очень понял эту секцию.

Suggested change
const [value, setValue] = React.useState(false);
React.useEffect(() => {
setValue(initial);
}, [initial]);
const [value, setValue] = React.useState(initial);

Мб я немного не понял intention, но зачем здесь useEffect, если initial можно сразу закинуть в useState ?

src/lib/box-styles.tsx Show resolved Hide resolved
</block.S>
)}
</State>
</Playground>
Copy link
Member

Choose a reason for hiding this comment

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

Нехватает ещё примеров использования Surface. Если сходу можешь придумать ещё парочку, было бы круто добавить.
Если нет, то просто резолвь этот комментарий

@tatinacher tatinacher marked this pull request as draft April 6, 2021 14:55
@tatinacher tatinacher marked this pull request as ready for review April 7, 2021 09:09
@sergeysova sergeysova merged commit 9cf7fba into master Apr 7, 2021
@sergeysova sergeysova deleted the feat/FRNT-366-fix-surface branch April 7, 2021 09:52
@sergeysova sergeysova removed lib documentation Improvements or additions to documentation labels Apr 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
@atoms Changes inside atoms
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants