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(experimental): add tuiButtonGroup #6686

Merged
merged 1 commit into from
Feb 12, 2024
Merged

feat(experimental): add tuiButtonGroup #6686

merged 1 commit into from
Feb 12, 2024

Conversation

splincode
Copy link
Member

@splincode splincode commented Feb 5, 2024 •
edited
Loading

No description provided.

@lumberjack-bot lumberjack-bot
Copy link

lumberjack-bot bot commented Feb 5, 2024 •
edited
Loading

Pull request was closed ✔️

All saved screenshots (for current PR) were deleted 🗑️

@splincode splincode added the v3 label Feb 5, 2024
@bundlemon BundleMon
Copy link

bundlemon bot commented Feb 5, 2024 •
edited
Loading

BundleMon

Files added (5)
Status Path Size Limits
✅ demo/browser/main.(hash).js
 +384.22KB +10%
✅ demo/browser/vendor.(hash).js
 +212.98KB +10%
✅ demo/browser/runtime.(hash).js
 +40.67KB +10%
✅ demo/browser/polyfills.(hash).js
 +11.25KB +10%
✅ demo/browser/styles.(hash).css
 +1.39KB +10%

Total files change +650.52KB

Groups added (1)
Status Path Size Limits
✅ demo/browser/*..js
 +2.36MB -

Final result: ✅

View report in BundleMon website ➡️

Current branch size history

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Feb 5, 2024 •
edited
Loading

Visit the preview URL for this PR (updated for commit 80e84b3):

https://taiga-ui--pr6686-splincode-button-gro-3zbzwcog.web.app

(expires Tue, 13 Feb 2024 13:21:44 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Sign: 4b5ece1e114386f6a105425ef799091475b249eb

@splincode splincode force-pushed the splincode/button-group-card branch from 53e44a8 to bb55522 Compare February 5, 2024 13:17
waterplea
waterplea reviewed Feb 5, 2024
View reviewed changes
projects/demo/src/modules/experimental/button-group-card/examples/1/index.html Outdated Show resolved Hide resolved
projects/demo/src/modules/experimental/button-group-card/examples/1/index.html Outdated Show resolved Hide resolved
projects/experimental/directives/button-group-card/button-group-card.style.less Outdated Show resolved Hide resolved
projects/experimental/directives/button-group-card/button-group-card.style.less Outdated Show resolved Hide resolved
projects/experimental/directives/button-group-card/button-group-card.style.less Outdated Show resolved Hide resolved
projects/experimental/directives/surface/surface.style.less Outdated Show resolved Hide resolved
projects/experimental/directives/surface/surface.style.less Outdated Show resolved Hide resolved
projects/experimental/directives/button-group-card/button-group-card.style.less Outdated Show resolved Hide resolved
projects/experimental/directives/button-group-card/button-group-card.style.less Outdated Show resolved Hide resolved
projects/experimental/directives/button-group-card/button-group-card.style.less Outdated Show resolved Hide resolved
@splincode splincode force-pushed the splincode/button-group-card branch from bb55522 to 6080d8a Compare February 6, 2024 08:15
@splincode splincode requested a review from waterplea February 6, 2024 08:16
@splincode splincode changed the title feat(experimental): add tuiButtonGroupCard feat(experimental): add tuiButtonGroup Feb 6, 2024
@splincode splincode force-pushed the splincode/button-group-card branch from d117e02 to ea8f4c1 Compare February 6, 2024 08:21
@codecov Codecov
Copy link

codecov bot commented Feb 6, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

❗ No coverage uploaded for pull request base (v3.x@db83218). Click here to learn what that means.

❗ Current head dcd7767 differs from pull request most recent head 80e84b3. Consider uploading reports for the commit 80e84b3 to get more accurate results

Additional details and impacted files 
@@           Coverage Diff           @@
##             v3.x    #6686   +/-   ##
=======================================
  Coverage        ?   70.58%           
=======================================
  Files           ?     1097           
  Lines           ?    13090           
  Branches        ?     1850           
=======================================
  Hits            ?     9240           
  Misses          ?     3539           
  Partials        ?      311
Flag Coverage Δ
addon-charts 71.46% <0.00%> (?)
addon-doc 60.58% <0.00%> (?)
addon-mobile 60.63% <0.00%> (?)
addon-table 60.75% <0.00%> (?)
addon-tablebars 96.15% <0.00%> (?)
cdk ∅ <0.00%> (?)
core 77.94% <0.00%> (?)
kit 70.28% <0.00%> (?)
summary 70.58% <0.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@waterplea
Copy link
Collaborator

waterplea commented Feb 6, 2024 •
edited
Loading

Just noticed that the icon is bigger than in vertical buttons. Let's not reuse vertical buttons, everything ends up different – padding, radius, gap, font. Let's just use regular button + tui-icon and apply all the necessary styles in the group.

@splincode splincode force-pushed the splincode/button-group-card branch 3 times, most recently from 41671c0 to 23e069a Compare February 7, 2024 09:30
@splincode
Copy link
Member Author

@waterplea updated

@splincode splincode force-pushed the splincode/button-group-card branch 2 times, most recently from 50c52d7 to 6abee23 Compare February 7, 2024 09:55
@splincode splincode force-pushed the splincode/button-group-card branch 2 times, most recently from 82b0230 to 151ab9c Compare February 7, 2024 15:55
@splincode splincode requested a review from waterplea February 7, 2024 15:55
waterplea
waterplea reviewed Feb 8, 2024
View reviewed changes
projects/demo/src/modules/experimental/button-group/examples/3/index.less Show resolved Hide resolved
projects/experimental/directives/button-group/button-group.style.less Outdated Show resolved Hide resolved
projects/experimental/directives/button-group/button-group.style.less Outdated Show resolved Hide resolved
projects/experimental/directives/button-group/button-group.style.less Outdated Show resolved Hide resolved
projects/experimental/directives/button-group/button-group.style.less Outdated
height: auto;
font-size: 0.8125rem;
white-space: pre-line;
min-width: 7.125rem;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need height, min-width and font-weight?

Copy link
Member Author

Choose a reason for hiding this comment

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

without min-width
image

Copy link
Collaborator

Choose a reason for hiding this comment

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

That's ok, we should just add width to the container, that is where it should come from.

projects/experimental/directives/button-group/button-group.style.less Show resolved Hide resolved
projects/experimental/directives/button-group/button-group.style.less Outdated Show resolved Hide resolved
projects/experimental/directives/button-group/button-group.style.less Outdated Show resolved Hide resolved
projects/experimental/directives/button-group/button-group.style.less Outdated Show resolved Hide resolved
projects/experimental/directives/button-group/button-group.style.less Outdated Show resolved Hide resolved
@waterplea
Copy link
Collaborator

Please also add font-size: 1.75rem to tui-icon

@splincode splincode force-pushed the splincode/button-group-card branch from 151ab9c to ecdd08d Compare February 8, 2024 11:06
@splincode splincode force-pushed the splincode/button-group-card branch from 0314f88 to dcd7767 Compare February 8, 2024 11:26
waterplea
waterplea reviewed Feb 8, 2024
View reviewed changes
projects/experimental/components/icon/icon.style.less Outdated Show resolved Hide resolved
projects/experimental/directives/button-group/button-group.style.less Outdated Show resolved Hide resolved
projects/experimental/directives/button-group/button-group.style.less Outdated Show resolved Hide resolved
projects/experimental/directives/button-group/button-group.style.less Outdated Show resolved Hide resolved
projects/experimental/directives/button-group/button-group.style.less Outdated Show resolved Hide resolved
projects/experimental/directives/button-group/button-group.style.less Outdated Show resolved Hide resolved
projects/experimental/directives/button-group/button-group.style.less Show resolved Hide resolved
projects/experimental/directives/button-group/button-group.style.less Outdated Show resolved Hide resolved
projects/experimental/directives/button-group/button-group.style.less Outdated Show resolved Hide resolved
projects/experimental/directives/button-group/button-group.style.less Outdated Show resolved Hide resolved
@splincode splincode force-pushed the splincode/button-group-card branch 3 times, most recently from aedb408 to 25c76f9 Compare February 8, 2024 16:57
waterplea
waterplea approved these changes Feb 12, 2024
View reviewed changes
projects/demo/src/modules/experimental/button-group/examples/1/index.less
}

[tuiButtonGroup] {
width: 21.5625rem;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why so specific? What if we move it to :host instead so there's no extra selectors in .less file?

@splincode splincode force-pushed the splincode/button-group-card branch from 25c76f9 to 80e84b3 Compare February 12, 2024 13:18
@vladimirpotekhin
Copy link
Member

nit: scrollbar in examples on imageiOS

@splincode splincode force-pushed the splincode/button-group-card branch from 80e84b3 to 60b9ea8 Compare February 12, 2024 13:39
@splincode splincode merged commit 03c4be3 into v3.x Feb 12, 2024
1 of 18 checks passed
@splincode splincode deleted the splincode/button-group-card branch February 12, 2024 13:39
@taiga-family-bot taiga-family-bot mentioned this pull request Feb 12, 2024
Merged
This was referenced Feb 20, 2024
Merged
Merged
@taiga-family-bot taiga-family-bot mentioned this pull request Mar 4, 2024
Merged
@taiga-family-bot taiga-family-bot mentioned this pull request Mar 12, 2024
Merged
This was referenced Mar 19, 2024
Merged
Merged
This was referenced Apr 2, 2024
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vladimirpotekhin vladimirpotekhin vladimirpotekhin approved these changes

@waterplea waterplea waterplea approved these changes

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

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

Assignees

@splincode splincode

Labels
ready to merge v3
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@splincode @waterplea @vladimirpotekhin