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

[FRNT-330] Rewrite Toggle, change name to Switch #75

Merged
merged 3 commits into from
Apr 15, 2021
Merged

[FRNT-330] Rewrite Toggle, change name to Switch #75

merged 3 commits into from
Apr 15, 2021

Conversation

tatinacher
Copy link
Member

Rewrite Toggle, change name to Switch.
Fix lib/keyboard-events to reuse in different components.
Update missed imports in ui.

https://liberty-base.atlassian.net/browse/FRNT-330

@tatinacher tatinacher added documentation Improvements or additions to documentation lib @molecules labels Apr 12, 2021
@github-actions github-actions bot added the @atoms Changes inside atoms label Apr 12, 2021
@tatinacher tatinacher force-pushed the fix/FRNT-330-rewrite-Toggle branch 2 times, most recently from 580c8eb to 718fe7a Compare April 12, 2021 10:28
Irinaristova
Irinaristova reviewed Apr 13, 2021
View reviewed changes
src/ui/molecules/switch/index.tsx Outdated
border-color: var(--woly-border-focus, var(--woly-background, #000000));
}

input:checked + span[data-checkbox] > span:before {
Copy link
Contributor

Choose a reason for hiding this comment

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

можно просто написать input:checked + span[data-checkbox] > span {
.....
&:before {....}
}

Copy link
Member Author

Choose a reason for hiding this comment

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

а в чем будет разница?

Copy link
Contributor

@Irinaristova Irinaristova Apr 14, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

лишняя строка уйдет, зачем дублировать + так более прослеживается код, как мне кажется

Copy link
Member Author

Choose a reason for hiding this comment

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

ага поняла про чо ты)

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
input:checked + span[data-checkbox] > span:before {
input:checked + [data-checkbox] > span:before {

мне кажется лучше убрать все эти span'ы

Второй селектор span:before вообще непонятно что выбирает. Какой span? Что он делает?

тут поможет вменяемый data-атрибут

src/ui/molecules/switch/usage.mdx Outdated Show resolved Hide resolved
src/ui/molecules/switch/index.tsx Outdated Show resolved Hide resolved
Irinaristova
Irinaristova reviewed Apr 14, 2021
View reviewed changes
src/ui/molecules/switch/index.tsx
@@ -0,0 +1,188 @@
import * as React from 'react';
import styled, { StyledComponent } from 'styled-components';
import { Variant } from 'lib/types';
Copy link
Contributor

Choose a reason for hiding this comment

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

я добавила экспорт Variant в index, чтобы можно одной строкой все экспортить

Irinaristova
Irinaristova reviewed Apr 14, 2021
View reviewed changes
src/ui/molecules/switch/index.tsx
}) => {
const tabIndex = disabled ? -1 : 0;

const onKeyDown = React.useCallback(
Copy link
Contributor

Choose a reason for hiding this comment

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

многим компонентам будет нужен этот handler, поэтому мб этот handler просто вынести и сюда импортить, чтобы не дублировать код в разных файлах?

sergeysova
sergeysova suggested changes Apr 14, 2021
View reviewed changes
src/lib/box-styles.tsx Outdated
@@ -7,6 +7,8 @@ export const Global = styled.div`
--woly-line-height: 24px;

[data-variant='primary'] {
--woly-border-width: 1.5px;
Copy link
Member

Choose a reason for hiding this comment

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

Это же не имеет отношения к варианту, это просто настройка.

src/lib/box-styles.tsx Show resolved Hide resolved
src/ui/molecules/switch/index.tsx Outdated
Comment on lines 82 to 89
--switcher-margin: calc(
var(--woly-switcher-height, 30px) / 2 -
var(--woly-switcher-tumbler-height, 24px) / 2
);
--switcher-toggle-margin: calc(
var(--woly-switcher-height, 30px) / 2 -
var(--woly-switcher-tumbler-height, 24px) / 2 - 1.5px
);
Copy link
Member

Choose a reason for hiding this comment

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

Я бы предложил сразу называть все локальные --local.
А вообще тут проблема, где определены все эти переменные switcher-height, switcher-tumbpler-height?

src/ui/molecules/switch/index.tsx Outdated
Comment on lines 114 to 120
width: var(--woly-switcher-width, 57px);
height: var(--woly-switcher-height, 30px);

background-color: var(--woly-canvas, #c0c0c0);
border-color: var(--woly-canvas, #c0c0c0);
border-style: solid;
border-radius: var(--woly-rounding, 18px);
Copy link
Member

Choose a reason for hiding this comment

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

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

src/ui/molecules/switch/index.tsx Outdated
Comment on lines 157 to 159
&:hover {
span[data-checkbox] > span,
input:checked + span[data-checkbox] > span {
Copy link
Member

Choose a reason for hiding this comment

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

Чет меня пугают такие селекторы, зачем так сложно?

src/ui/molecules/switch/index.tsx Outdated
border-color: var(--woly-border-focus, var(--woly-background, #000000));
}

input:checked + span[data-checkbox] > span:before {
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
input:checked + span[data-checkbox] > span:before {
input:checked + [data-checkbox] > span:before {

мне кажется лучше убрать все эти span'ы

Второй селектор span:before вообще непонятно что выбирает. Какой span? Что он делает?

тут поможет вменяемый data-атрибут

src/ui/molecules/switch/index.tsx Outdated
}

label > *:not(:first-child) {
margin-left: var(--woly-gap);
Copy link
Member

Choose a reason for hiding this comment

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

А где определение свойства --woly-gap?

src/ui/molecules/switch/index.tsx Outdated
}
}

&:focus > label > span[data-checkbox] > span {
Copy link
Member

Choose a reason for hiding this comment

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

А реально имеет смысл такая вложенность?
нельзя проще?

&:focus [data-checkbox] span {

Или вообще сразу дать имя этому span'у и указать на него

Irinaristova
Irinaristova reviewed Apr 14, 2021
View reviewed changes
src/ui/molecules/switch/index.tsx Outdated

input:checked + span[data-checkbox] > span {
background-color: var(--woly-background, #b0a3f4);
border-color: var(--woly-border-focus, var(--woly-background, #000000));
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
Member

Choose a reason for hiding this comment

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

Вообще, нет.
Поэтому любые дефолты по хорошему нужно выпилить

src/ui/molecules/switch/index.tsx Outdated
import { keyboardEventHandle } from 'lib';

/**
* --woly-switcher-width
Copy link
Contributor

Choose a reason for hiding this comment

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

это можно уже удалять наверно

@sergeysova sergeysova removed @molecules documentation Improvements or additions to documentation lib labels Apr 14, 2021
@sergeysova sergeysova merged commit 5899b85 into master Apr 15, 2021
@sergeysova sergeysova deleted the fix/FRNT-330-rewrite-Toggle branch April 15, 2021 12:51
@sergeysova sergeysova added feature and removed lib labels Apr 15, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@sergeysova sergeysova Awaiting requested review from sergeysova

@Irinaristova Irinaristova Awaiting requested review from Irinaristova

Assignees
No one assigned
Labels
@atoms Changes inside atoms feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@tatinacher @sergeysova @Irinaristova