Skip to content

feat(Placeholder): tokenized#2610

Merged
SevereCloud merged 3 commits intomasterfrom
SevereCloud/issue2559
Jul 13, 2022
Merged

feat(Placeholder): tokenized#2610
SevereCloud merged 3 commits intomasterfrom
SevereCloud/issue2559

Conversation

@SevereCloud
Copy link
Copy Markdown
Contributor

@SevereCloud SevereCloud commented May 25, 2022 •
edited by inomdzhon
Loading

Чеклист перевода компонента на vkui-tokens

  • В стилях компонента не осталось платформенных селекторов
  • Если в стилях встречаются токены из Appearance, то их нужно не удалять, а дополнять фоллбэком на соответствующий токен из vkui-tokens
  • В tsx компонента не осталось логики, которая зависит от платформы
  • Компонент добавлен в src/tokenized/index.ts (в src/index.ts он так же должен быть)
  • Имя компонента добавлено в массив из styleguide/tokenized.js
  • Дизайн ревью

Документация компонентов

Скриншотный тест

тест

@SevereCloud SevereCloud requested a review from a team as a code owner May 25, 2022 16:55
@SevereCloud SevereCloud mentioned this pull request May 25, 2022
Closed
89 tasks
@codesandbox-ci
Copy link
Copy Markdown

codesandbox-ci bot commented May 25, 2022 •
edited
Loading

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 6935dac:

Sandbox Source
VKUI - default example Configuration

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented May 25, 2022 •
edited
Loading

size-limit report 📦

Path Size
JS 259.62 KB (-0.01% 🔽)
JS (gzip) 76.92 KB (-0.01% 🔽)
JS (brotli) 64.89 KB (-0.05% 🔽)
JS, unstable 30.05 KB (0%)
CSS 278.71 KB (+0.02% 🔺)
CSS (gzip) 39.15 KB (-0.01% 🔽)
CSS (brotli) 31.41 KB (+0.02% 🔺)
CSS, unstable 960 B (0%)

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented May 25, 2022

👀 Styleguide deployed

See the styleguide for this PR at https://vkcom.github.io/VKUI/pull/2610/

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented May 25, 2022 •
edited
Loading

Code coverage

lines3798 / 486078.14%
statements3867 / 495578.04%
functions843 / 106079.52%
branches3341 / 466471.63%
branchesTrue0 / 0100.00%

Generated by 🚫 dangerJS against 6935dac

eugpoloz
eugpoloz reviewed May 26, 2022
View reviewed changes
src/components/Placeholder/Placeholder.css

.Placeholder__action .Button--lvl-tertiary {
top: -8px;
margin-top: 16px;
Copy link
Copy Markdown
Contributor

@eugpoloz eugpoloz May 26, 2022

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor Author

@SevereCloud SevereCloud May 26, 2022

Choose a reason for hiding this comment

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

Нет вроде

@SevereCloud SevereCloud force-pushed the SevereCloud/issue2559 branch from 55737a1 to 000587a Compare May 26, 2022 11:57
@stoope
Copy link
Copy Markdown
Contributor

stoope commented May 30, 2022

Давай еще добавим скриншотных тестов для ButtonGroup внутри Placeholder

@SevereCloud
Copy link
Copy Markdown
Contributor Author

SevereCloud commented May 30, 2022

Давай еще добавим скриншотных тестов для ButtonGroup внутри Placeholder

VKUI/src/components/Placeholder/Placeholder.e2e.tsx

Lines 11 to 16 in 000587a

<ButtonGroup key="button-group" mode="vertical">
<Button size="m">Button</Button>
<Button size="m" mode="tertiary">
Button
</Button>
</ButtonGroup>,

Они уже есть

stoope
stoope previously approved these changes May 30, 2022
View reviewed changes
inomdzhon
inomdzhon reviewed May 31, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@inomdzhon inomdzhon left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Воу, мб не надо так?

.Placeholder__action .Button:not(:first-child)

.Placeholder__action .ButtonGroup

Вместо этого можно сделать так

<ButtonGroup mode="vertical" gap="m">
 <Button>Lorem ipsum</Button>
 <Button mode="tertiary" stretched>Lorem</Button>
</ButtonGroup>

image

  1. gap="m" задаёт margin-top: 12px и решает проблему с .Placeholder__action .Button:not(:first-child)
  2. Button mode="tertiary" stretched решает проблему .Placeholder__action .ButtonGroup

Мы пытаемся сократить обращения из одного блока к другому. Здесь, вижу, можно избежать это.

inomdzhon
inomdzhon requested changes May 31, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@inomdzhon inomdzhon left a comment •
edited
Loading

Choose a reason for hiding this comment

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

см. #2610 (review)

+ потом надо поправить доку и добавить пример с ButtonGroup

@SevereCloud
Copy link
Copy Markdown
Contributor Author

SevereCloud commented May 31, 2022

2. gap="m" задаёт margin-top: 12px

Нужно 16px

Макет image

Вообще в дизайне нет ButtonGroup, только две кнопки и между ними 16px. Как вариант могу избавиться от поддержки ButtonGroup и задать gap

@inomdzhon
Copy link
Copy Markdown
Contributor

inomdzhon commented May 31, 2022

Вообще в дизайне нет ButtonGroup, только две кнопки и между ними 16px. Как вариант могу избавиться от поддержки ButtonGroup и задать gap

@SevereCloud я вижу ты в Figma спросил можно ли ButtonGroup использовать и пока не ответили. Давай пинганём и дождёмся ответа?

@SevereCloud
Copy link
Copy Markdown
Contributor Author

SevereCloud commented May 31, 2022

Ок

@SevereCloud SevereCloud force-pushed the SevereCloud/issue2559 branch from 000587a to 0d1938f Compare June 28, 2022 15:06
inomdzhon
inomdzhon reviewed Jun 28, 2022
View reviewed changes
@SevereCloud SevereCloud force-pushed the SevereCloud/issue2559 branch from eeb2b30 to f183be6 Compare June 30, 2022 17:12
@SevereCloud SevereCloud force-pushed the SevereCloud/issue2559 branch from c706467 to 41f6e5e Compare July 8, 2022 13:07
@SevereCloud SevereCloud force-pushed the SevereCloud/issue2559 branch from 4749e70 to 6935dac Compare July 13, 2022 12:11
inomdzhon
inomdzhon approved these changes Jul 13, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@inomdzhon inomdzhon left a comment

Choose a reason for hiding this comment

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

👍

@SevereCloud
Copy link
Copy Markdown
Contributor Author

SevereCloud commented Jul 13, 2022

Дизайн ревью ✅ (by @Zaycevq)

@SevereCloud SevereCloud merged commit 44d0fb2 into master Jul 13, 2022
@SevereCloud SevereCloud deleted the SevereCloud/issue2559 branch July 13, 2022 13:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@inomdzhon inomdzhon inomdzhon approved these changes

+3 more reviewers

@eolme eolme eolme left review comments

@eugpoloz eugpoloz eugpoloz left review comments

@stoope stoope stoope left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Feature] Токенизировать Placeholder

5 participants

@SevereCloud @stoope @inomdzhon @eolme @eugpoloz