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.

Sign up for GitHub

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

Jump to bottom

feat(segmented-control): add component #528

Merged
merged 3 commits into from
Feb 28, 2023
Merged

feat(segmented-control): add component #528

merged 3 commits into from
Feb 28, 2023

Conversation

AlexZJScript
Copy link
Contributor

Добавление компонента SegmentedControl

@changeset-bot
Copy link

changeset-bot bot commented Feb 12, 2023 •
edited

🦋 Changeset detected

Latest commit: 87d1bfe

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@alfalab/core-components-segmented-control Major

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@coveralls
Copy link

coveralls commented Feb 12, 2023 •
edited

Pull Request Test Coverage Report for Build 4204832423

  • 58 of 59 (98.31%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.07%) to 79.961%
Changes Missing Coverage Covered Lines Changed/Added Lines %
packages/segmented-control/src/context.ts 2 3 66.67%
Totals Coverage Status
Change from base Build 4202848833: 0.07%
Covered Lines: 7652
Relevant Lines: 8688
💛 - Coveralls

@core-ds-bot
Copy link
Collaborator

Собрана новая демка.

@SiebenSieben
Copy link
Contributor

Changeset

v-gevak
v-gevak reviewed Feb 13, 2023
View reviewed changes
packages/segmented-control/src/Component.tsx Outdated
const selectedBoxRef = useRef<HTMLDivElement>(null);

useEffect(() => {
handler.current = onChange;
Copy link
Contributor

Choose a reason for hiding this comment

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

Не понял чего ты этим добился. Замемоизировал контекст? А смысл?

v-gevak
v-gevak suggested changes Feb 13, 2023
View reviewed changes
packages/segmented-control/src/Component.tsx Outdated
contentClassName:
isPositionFounded && children[selectedSegmentPosition].props.contentClassName,
};
}, [selectedId, children]);
Copy link
Contributor

Choose a reason for hiding this comment

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

children всегда новые будут, этот useMemo никогда не будет работать

packages/segmented-control/src/Component.tsx Outdated
selectedBoxRef.current.style.width = `${width}px`;
selectedBoxRef.current.style.transform = `translateX(${selectedSegment.offsetLeft}px)`;
}
}, [children, selectedId]);
Copy link
Contributor

Choose a reason for hiding this comment

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

Тоже самое, из-за children useEffect будет на каждый рендер срабатывать

packages/segmented-control/src/Component.tsx Outdated
</div>
</div>
</div>
<div className={cn(contentClassName)}>{content}</div>
Copy link
Contributor

Choose a reason for hiding this comment

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

content && <div.... Контента ведь может и не быть

packages/segmented-control/src/index.module.css Outdated
}

.xs {
height: 40px;
Copy link
Contributor

Choose a reason for hiding this comment

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

Заюзать переменную --size-xs-height

packages/segmented-control/src/index.module.css Outdated
}

.xxs {
height: 32px;
Copy link
Contributor

Choose a reason for hiding this comment

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

Заюзать переменную --size-xxs-height

packages/segmented-control/src/docs/description.mdx

## Количество сегментов

Контрол может содержать от двух до пяти сегментов.
Copy link
Contributor

Choose a reason for hiding this comment

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

А если 6 указать?))

Copy link
Contributor

Choose a reason for hiding this comment

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

Должно ли быть ограничение или нет? Если нет, то не ясно зачем в описании так написано

Copy link
Contributor

Choose a reason for hiding this comment

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

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

SiebenSieben
SiebenSieben reviewed Feb 15, 2023
View reviewed changes
packages/segmented-control/package.json Outdated
@@ -0,0 +1,25 @@
{
"name": "@alfalab/core-components-segmented-control",
"version": "1.0.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

0.0.0

SiebenSieben
SiebenSieben reviewed Feb 15, 2023
View reviewed changes
packages/segmented-control/src/Component.tsx Outdated
/**
* Вид компонента
*/
view?: 'rounded' | 'rect';
Copy link
Contributor

Choose a reason for hiding this comment

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

Сейчас у Tag есть shape (rounded/rectangular), может тут сделаем аналогично?

@v-gevak
Copy link
Contributor

v-gevak commented Feb 15, 2023

Тесты нужно добавить

v-gevak
v-gevak suggested changes Feb 15, 2023
View reviewed changes
packages/segmented-control/src/Component.tsx
useEffect(() => {
if (!wrapperRef.current) return undefined;

const observer = new ResizeObserver(() => setSelectedBoxStylesRef.current());
Copy link
Contributor

Choose a reason for hiding this comment

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

Нужно полифил заиспользовать. Можно в других компонентах посмотреть как сделано

packages/segmented-control/src/docs/description.mdx

## Количество сегментов

Контрол может содержать от двух до пяти сегментов.
Copy link
Contributor

Choose a reason for hiding this comment

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

Должно ли быть ограничение или нет? Если нет, то не ясно зачем в описании так написано

@core-ds-bot
Copy link
Collaborator

Собрана новая демка.

@core-ds-bot
Copy link
Collaborator

Собрана новая демка.

@core-ds-bot
Copy link
Collaborator

Собрана новая демка.

@core-ds-bot
Copy link
Collaborator

Собрана новая демка.

@v-gevak
Copy link
Contributor

v-gevak commented Feb 17, 2023

Скриншоты бы еще)

@core-ds-bot
Copy link
Collaborator

Собрана новая демка.

@Oladii
Copy link
Contributor

Oladii commented Feb 17, 2023

На всю ширину тянем только в мобильной демке
image

@AlexZJScript
Copy link
Contributor Author

На всю ширину тянем только в мобильной демке image

fixed

@core-ds-bot
Copy link
Collaborator

Собрана новая демка.

@core-ds-bot
Copy link
Collaborator

Собрана новая демка.

@SiebenSieben SiebenSieben merged commit 1dd5745 into master Feb 28, 2023
@SiebenSieben SiebenSieben deleted the feat/segmented-control branch February 28, 2023 09:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SiebenSieben SiebenSieben SiebenSieben approved these changes

@v-gevak v-gevak v-gevak approved these changes

@Oladii Oladii Oladii approved these changes

@reme3d2y reme3d2y Awaiting requested review from reme3d2y reme3d2y is a code owner

@EGNKupava EGNKupava Awaiting requested review from EGNKupava

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@AlexZJScript @coveralls @core-ds-bot @SiebenSieben @v-gevak @Oladii