Skip to content

Deprecate Separator#2359

Merged
eugpoloz merged 4 commits intomasterfrom
enhancement/use-spacing
May 23, 2022
Merged

Deprecate Separator#2359
eugpoloz merged 4 commits intomasterfrom
enhancement/use-spacing

Conversation

@SevereCloud
Copy link
Copy Markdown
Contributor

@SevereCloud SevereCloud commented Apr 7, 2022 •
edited
Loading

@SevereCloud SevereCloud requested a review from a team as a code owner April 7, 2022 07:54
@SevereCloud SevereCloud mentioned this pull request Apr 7, 2022
Closed
11 tasks
@codesandbox-ci
Copy link
Copy Markdown

codesandbox-ci bot commented Apr 7, 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 537e297:

Sandbox Source
VKUI - default example Configuration

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 7, 2022 •
edited
Loading

size-limit report 📦

Path Size
JS 76.92 KB (-0.01% 🔽)
JS, unstable 26.24 KB (0%)
CSS 43.12 KB (-0.01% 🔽)
CSS, unstable 1015 B (0%)

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 7, 2022

👀 Styleguide deployed

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

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 7, 2022 •
edited
Loading

Code coverage

lines3723 / 475478.31%
statements3794 / 484678.29%
functions842 / 104580.57%
branches3093 / 440170.27%
branchesTrue0 / 0100.00%

Generated by 🚫 dangerJS against 537e297

@SevereCloud
Copy link
Copy Markdown
Contributor Author

SevereCloud commented Apr 7, 2022

image
Думаю, что высоту надо бы подправить

@eugpoloz
Copy link
Copy Markdown
Contributor

eugpoloz commented Apr 7, 2022

@SevereCloud да, хорошо бы подогнать!

@SevereCloud SevereCloud force-pushed the enhancement/use-spacing branch from 662eef6 to 1916851 Compare April 7, 2022 16:25
@inomdzhon
Copy link
Copy Markdown
Contributor

inomdzhon commented Apr 8, 2022 •
edited
Loading

Можешь, плиз, сразу поправить // eslint-disable-next-line vkui/no-object-expression-in-arguments в файлах, которые ты затронул? По diff'у я 8 вхождениq насчитал

@SevereCloud SevereCloud force-pushed the enhancement/use-spacing branch from 1916851 to d578f80 Compare April 8, 2022 09:41
inomdzhon
inomdzhon requested changes Apr 8, 2022
View reviewed changes
@SevereCloud SevereCloud force-pushed the enhancement/use-spacing branch from d578f80 to 3fc0bb6 Compare April 8, 2022 09:46
@SevereCloud
Copy link
Copy Markdown
Contributor Author

SevereCloud commented Apr 8, 2022 •
edited
Loading

Насчитал 6 + 3 classNames в стайлгаде

@inomdzhon
Copy link
Copy Markdown
Contributor

inomdzhon commented Apr 8, 2022

Сейчас на стайлгад не распространяется это правило vkui/no-object-expression-in-arguments, поэтому там не обязательно менять в дальнейшем
Но обсуждаемо

This was referenced Apr 12, 2022
Merged
Closed
eugpoloz
eugpoloz suggested changes Apr 12, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@eugpoloz eugpoloz left a comment

Choose a reason for hiding this comment

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

Склонна согласиться с комментом Игоря из предыдущего PR: замена <Separator wide /> на border не выглядит полноценной. Предлагаю посоветоваться с дизайнерами.

@fedorov-xyz
Copy link
Copy Markdown
Contributor

fedorov-xyz commented Apr 12, 2022 •
edited
Loading

А напомните, почему решили отказаться от <Separator />? Кажется, что оба этих компонента могут существовать рядом. К тому же Separator более легковесный, чем <Spacing /> с какой-то логикой и даже flex-контейнером внутри.

Во многих местах типа как в документации со Spacing придётся костылить, заменяя сепаратор на него.

@eugpoloz
Copy link
Copy Markdown
Contributor

eugpoloz commented Apr 12, 2022

@fedorov-xyz я не в курсе, к сожалению, только увидела, что в доке его уже не советуют использовать и что на его замену должен прийти Spacing. Поэтому и предлагаю сходить к дизайну и поднять вопрос, мне тоже кажется, что сейчас они как-то не очень взаимозаменяемы.

@fedorov-xyz
Copy link
Copy Markdown
Contributor

fedorov-xyz commented Apr 12, 2022

Пока что выглядит, будто в доку написали случайно (?). И хотя уже много работы сделано в этом PR, я бы предложил не спешить с выпиливанием сепаратора.

@SevereCloud SevereCloud marked this pull request as draft April 13, 2022 05:39
@eugpoloz eugpoloz added the design Нужно участие команды дизайна label Apr 20, 2022
@SevereCloud SevereCloud force-pushed the enhancement/use-spacing branch 2 times, most recently from b98d0d3 to b9238ce Compare May 19, 2022 10:33
@SevereCloud SevereCloud marked this pull request as ready for review May 19, 2022 10:41
eugpoloz
eugpoloz reviewed May 19, 2022
View reviewed changes
@SevereCloud SevereCloud force-pushed the enhancement/use-spacing branch from b9238ce to 537e297 Compare May 19, 2022 11:58
@SevereCloud SevereCloud requested a review from eugpoloz May 19, 2022 15:53
@eugpoloz eugpoloz merged commit 5f7686a into master May 23, 2022
@eugpoloz eugpoloz deleted the enhancement/use-spacing branch May 23, 2022 07:42
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

+1 more reviewer

@eugpoloz eugpoloz eugpoloz approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

design Нужно участие команды дизайна

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Enhancement] Задепрекейтить излишние свойства в Separator и Spacing

4 participants

@SevereCloud @eugpoloz @inomdzhon @fedorov-xyz