feat: button group #2473

maksim-karatkevich · 2020-08-05T20:49:08Z

Please read and mark the following check list before creating a pull request:

I read and followed the CONTRIBUTING.md guide.
I read and followed the New Feature Checklist guide.

Short description of what this resolves:

codecov · 2020-08-05T21:08:06Z

Codecov Report

Merging #2473 (738e86d) into master (b7a2c6e) will decrease coverage by 0.34%.
The diff coverage is 62.41%.

@@            Coverage Diff             @@
##           master    #2473      +/-   ##
==========================================
- Coverage   78.93%   78.59%   -0.35%     
==========================================
  Files         253      258       +5     
  Lines        7815     7908      +93     
  Branches      857      875      +18     
==========================================
+ Hits         6169     6215      +46     
- Misses       1382     1427      +45     
- Partials      264      266       +2

Impacted Files	Coverage Δ
...omponents/button-toggle/button-toggle.directive.ts	`40.00% <40.00%> (ø)`
.../components/button-group/button-group.component.ts	`53.33% <53.33%> (ø)`
...k/theme/components/button/base-button.directive.ts	`74.62% <74.62%> (ø)`
...eme/components/button-group/button-group.module.ts	`100.00% <100.00%> (ø)`
...e/components/button-toggle/button-toggle.module.ts	`100.00% <100.00%> (ø)`
...mework/theme/components/button/button.component.ts	`81.39% <100.00%> (-2.88%)`	⬇️
... and 1 more

ghost · 2020-09-10T14:07:42Z

DeepCode's analysis on #0d88ee found:

ℹ️ 4 minor issues. 👇
✔️ 1 issue was fixed.

Top issues

Description	Example fixes
Array type using 'T[]' is forbidden for non-simple types. Use 'Array' instead. Occurrences: button-group.component.ts:232	🔧 Example fixes
asterisks in jsdoc must be aligned Occurrences: button-group.component.ts:86 base-button.ts:212 button-toggle.directive.ts:53	🔧 Example fixes
Type boolean trivially inferred from a boolean literal, remove type annotation Occurrences: button-group.component.ts:145 base-button.ts:119 button-group.spec.ts:43	🔧 Example fixes

👉 View analysis in DeepCode’s Dashboard | Configure the bot

docs/structure.ts

src/playground/with-layout/button/button.module.ts

docs/assets/images/components/button-group.svg

docs/app/@theme/styles/styles.scss

src/framework/theme/components/button-group/_button-group.component.theme.scss

src/framework/theme/styles/themes/_mapping.scss

…ton-group

fix: changed way button styles connect fix: removed unused imports and styles

yggg

A few more comments:

Use default sized button in examples (except for size example). Currently most example has giant buttons for some reason.
button-group-multiple.component example. Icon only buttons should be square shaped (see example here). Make sure button toggle also has same styles as button here.
Ensure button and button toggle has the same dimensions. You can add button toggles to the button interactive examples and compare it there. https://akveo.github.io/nebular/example/button/button-interactive.component. And also check button dimensions didn't changed in this PR (compare with online docs examples).

docs/app/@theme/styles/styles.scss

yggg · 2020-10-02T12:08:34Z

src/framework/theme/components/button-group/_button-group.component.theme.scss

+    div {
+      display: flex;
+      align-items: center;
+    }


There is no div in the nb-button-group template.

yggg · 2020-10-02T12:12:39Z

src/framework/theme/components/button-group/_button-group.component.theme.scss

+
+    [nbButtonToggle], [nbButton] {
+      &:focus {
+        //Needs to make shadow visible


Let's clarify with something like:

Sibling buttons are covering outline shadow. position: relative; declaration makes shadow appear on top of sibling buttons.

yggg · 2020-10-05T09:41:03Z

src/framework/theme/components/button-group/_button-group.component.theme.scss

+              border-radius, 0 nb-theme(button-#{$shape}-border-radius) nb-theme(button-#{$shape}-border-radius) 0);
+          @include nb-rtl(
+              border-radius, nb-theme(button-#{$shape}-border-radius) 0 0 nb-theme(button-#{$shape}-border-radius));
+        }
+
+        &:first-child.shape-#{$shape} {
+          @include nb-ltr(
+              border-radius, nb-theme(button-#{$shape}-border-radius) 0 0 nb-theme(button-#{$shape}-border-radius));
+          @include nb-rtl(
+              border-radius, 0 nb-theme(button-#{$shape}-border-radius) nb-theme(button-#{$shape}-border-radius) 0);


Assign nb-theme(button-#{$shape}-border-radius) to a variable and use it to reduce lines length.

yggg · 2020-10-05T09:44:31Z

src/framework/theme/components/button-group/_button-group.component.theme.scss

+            @include nb-ltr(border-right,
+              nb-theme(button-toggle-border-width)
+              nb-theme(button-toggle-border-style)
+              nb-theme(button-toggle-border-color));
+            @include nb-rtl(border-left,
+              nb-theme(button-toggle-border-width)
+              nb-theme(button-toggle-border-style)
+              nb-theme(button-toggle-border-color));
+
+            &:hover {
+              @include nb-ltr(border-right,
+                nb-theme(button-toggle-border-width)
+                nb-theme(button-toggle-border-style)
+                nb-theme(button-toggle-border-color));
+              @include nb-rtl(border-left,
+                nb-theme(button-toggle-border-width)
+                nb-theme(button-toggle-border-style)
+                nb-theme(button-toggle-border-color));


Let's introduce a variable and set it to border values so we don't need to repeat tree nb-theme calls four times. Something like:

$border: nb-theme(button-toggle-border-width) nb-theme(button-toggle-border-style) nb-theme(button-toggle-border-color); // ... @include nb-ltr(border-right, $border);

src/framework/theme/components/button-group/button-group.component.ts

src/playground/with-layout/button-group/button-group.module.ts

Instead of setting border-radius, reset it only on unnecessary element sides. Correct border-radius already set in button and button toggle styles. We only need to reset it to 0 on the sides where the element has siblings.

…t/button-group

Don't remove border as height of buttons inside the group would be smaller than buttons outside the group. Also, make divider on the start side as per design.

(cherry picked from commit 72bb1b3)

maksim-karatkevich requested a review from yggg August 5, 2020 20:49

Maksim Karatkevich added 10 commits August 18, 2020 17:57

feat(button-group): add button-group.component

c6def0c

feat(button-group): fix issues, add docs

f11382c

fix(tabIndex): fix linter

10aedab

feat(button-group): update docs

afe383f

feat(button-group): fix styles

2a4bda1

feat(button-group): fix lint

bb29543

feat(button-group): fix lint

abd9aaa

feat(button-group): fix comments

95ea86e

feat(button-group): add base button component

5329d11

fix(tabIndex): fix lint

df92134

maksim-karatkevich force-pushed the feat/button-group branch from b743873 to df92134 Compare August 18, 2020 15:01

fix(tabIndex): fix errors

52752aa

yggg changed the title ~~Feat/button group~~ feat: button group Sep 1, 2020

maksim-karatkevich and others added 2 commits September 3, 2020 13:49

Merge branch 'master' into feat/button-group

91c7a00

Merge branch 'master' into feat/button-group

7e58c7e

yggg suggested changes Sep 10, 2020

View reviewed changes

Maksim Karatkevich and others added 5 commits September 11, 2020 19:06

feat(button-group): fix comments

5e64b9f

Merge remote-tracking branch 'origin/feat/button-group' into feat/but…

af713a0

…ton-group

refactor: reorganized buttons structure

d894742

fix: changed way button styles connect fix: removed unused imports and styles

fix: fixed linter issues

8f67ad4

Merge branch 'master' into feat/button-group

b403d65

yggg suggested changes Oct 5, 2020

View reviewed changes

Sergey Filinsky and others added 5 commits October 6, 2020 21:37

fix: fixed pull request comments

0532558

fix: fixed linter issues

f86a215

fix: fixed button sizes

1749ae9

fix: improved button group styles

1804a34

Merge branch 'master' into feat/button-group

b50573b

yggg and others added 24 commits October 15, 2020 18:44

refactor(button group): make inline flex

34879d9

style(button group): change formatting

cac7647

refactor(button group): don't set border radius

8e8638f

Instead of setting border-radius, reset it only on unnecessary element sides. Correct border-radius already set in button and button toggle styles. We only need to reset it to 0 on the sides where the element has siblings.

refactor(button group): make appearance an input

d6840dc

style(button group): update formatting and members order

69b1f3d

docs(button group): remove unrelated theme properties

fa401e0

fix(button group): correct border radius for single group member

1cf4aa2

refactor(button toggle): simplify status class bindings

b364d3e

fix(button group): button text same color as toggle color

a4481ab

Merge branch 'master' into feat/button-group

e13be5a

Merge branch 'feat/button-group' of github.com:akveo/nebular into fea…

738e86d

…t/button-group

Merge branch 'feat/button-group-beta' into feat/button-group

abbb0d2

refactor(group): remove unnecessary import

e5acf49

refactor(group): enable OnPush cd strategy

839e845

fix(toggle): mark for check when changing pressed state from group

013d070

refactor(group): determine button type via instaceof

e9e72c5

refactor(group): rename method to clarify what it does

5ece357

refactor(button): move tabindex related properties to the base class

4b47978

refactor(button): remove duplicate properties defined in a base class

d24d8aa

style: revert accidental changes

6da91f6

refactor: remove comment

34c6f75

test(group): refactor and add tests for a newly added group

c5b5662

fix(group): defer update changed projected content

de460d5

fix(group): correct buttons height

0d88ee5

Don't remove border as height of buttons inside the group would be smaller than buttons outside the group. Also, make divider on the start side as per design.

yggg force-pushed the feat/button-group branch from 8dea161 to 0d88ee5 Compare December 16, 2020 15:01

yggg approved these changes Dec 16, 2020

View reviewed changes

yggg merged commit 72bb1b3 into master Dec 16, 2020

yggg deleted the feat/button-group branch December 16, 2020 15:04

yggg restored the feat/button-group branch December 16, 2020 15:04

yggg pushed a commit that referenced this pull request Dec 24, 2020

feat: button group (#2473)

b2ace46

(cherry picked from commit 72bb1b3)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: button group #2473

feat: button group #2473

maksim-karatkevich commented Aug 5, 2020

codecov bot commented Aug 5, 2020 •

edited

Loading

ghost commented Sep 10, 2020 •

edited by ghost

Loading

yggg left a comment

yggg Oct 2, 2020

yggg Oct 2, 2020

yggg Oct 5, 2020

yggg Oct 5, 2020

feat: button group #2473

feat: button group #2473

Conversation

maksim-karatkevich commented Aug 5, 2020

Please read and mark the following check list before creating a pull request:

Short description of what this resolves:

codecov bot commented Aug 5, 2020 • edited Loading

Codecov Report

ghost commented Sep 10, 2020 • edited by ghost Loading

DeepCode's analysis on #0d88ee found:

Top issues

👉 View analysis in DeepCode’s Dashboard | Configure the bot

yggg left a comment

Choose a reason for hiding this comment

yggg Oct 2, 2020

Choose a reason for hiding this comment

yggg Oct 2, 2020

Choose a reason for hiding this comment

yggg Oct 5, 2020

Choose a reason for hiding this comment

yggg Oct 5, 2020

Choose a reason for hiding this comment

codecov bot commented Aug 5, 2020 •

edited

Loading

ghost commented Sep 10, 2020 •

edited by ghost

Loading