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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Stories rework: VaButton #3890

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from
Open

Stories rework: VaButton #3890

wants to merge 6 commits into from

Conversation

Roman4437
Copy link
Collaborator

No description provided.

asvae
asvae requested changes Sep 21, 2023
View reviewed changes
Copy link
Contributor

@asvae asvae left a comment •
edited

Choose a reason for hiding this comment

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

Pretty close to finish on stories. I'd like to get some output from Maksim on useSize before we continue though.

We also have some test regressions.

packages/ui/src/components/va-button/VaButton.stories.ts Outdated Show resolved Hide resolved
packages/ui/src/components/va-button/VaButton.stories.ts Show resolved Hide resolved
packages/ui/src/components/va-button/VaButton.stories.ts Show resolved Hide resolved
packages/ui/src/components/va-button/VaButton.stories.ts Outdated Show resolved Hide resolved
packages/ui/src/components/va-button/VaButton.stories.ts Show resolved Hide resolved
packages/ui/src/components/va-button/VaButton.stories.ts Show resolved Hide resolved
packages/ui/src/components/va-button/VaButton.stories.ts Show resolved Hide resolved
@asvae
Copy link
Contributor

asvae commented Sep 21, 2023

@m0ksem do you have some feedback to share for useSize?

@asvae asvae mentioned this pull request Sep 23, 2023
Open
75 tasks
@asvae
Copy link
Contributor

asvae commented Sep 23, 2023 •
edited

That would be the last round of feedback from me. We're good to merge after that 馃槑

@asvae asvae requested a review from m0ksem October 12, 2023 12:59
asvae
asvae approved these changes Oct 12, 2023
View reviewed changes
Copy link
Contributor

@asvae asvae left a comment •
edited

Choose a reason for hiding this comment

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

Stories are fine from my side

css and sizes updates need review from @m0ksem

m0ksem
m0ksem approved these changes Oct 15, 2023
View reviewed changes
Copy link
Contributor

@m0ksem m0ksem left a comment

Choose a reason for hiding this comment

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

I think we will polish sizes config later with @xx13 when he's gonna be ready.
So, you can change sizes as you want until it doesn't produce breaking changes.

packages/ui/src/components/va-button/VaButton.vue

.va-button__content {
font-size: var(--va-button-font-size);
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove CSS variables too.

Copy link
Contributor

@asvae asvae Nov 13, 2023 •
edited

Choose a reason for hiding this comment

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

Just checked - these variables are used in a couple of components, i.e. pagination and upload list. So might be something for different PR.

asvae
asvae requested changes Nov 22, 2023
View reviewed changes
Copy link
Contributor

@asvae asvae left a comment

Choose a reason for hiding this comment

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

Everything button related is finished, but css variables removal is incomplete.

So I believe the best solution would be not to touch it at all for now.

So we need to rollback css changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@asvae asvae asvae requested changes

@m0ksem m0ksem m0ksem approved these changes

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
3 participants
@Roman4437 @asvae @m0ksem