feat: vertical oriented button group #755

ruanyl · 2023-05-10T04:44:01Z

Description

This commit added a new prop(orientation) to the current OuiButtonGroup, the possible values are horizontal and vertical. The default value is horizontal which match the current UI behavior. With vertical prop, the button group will be oriented vertically.

Example:

Issues Resolved

#659

Check List

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

For more information on following Developer Certificate of Origin and signing off your commits, please check here.

ashwin-pc

@ruanyl Nice! just a few questions

ashwin-pc · 2023-05-10T15:54:20Z

src/components/button/button_group/_button_group_button.scss

@@ -123,20 +123,43 @@
 .ouiButtonGroup--small {
  .ouiButtonGroupButton {
    border: $ouiBorderThin;
+  }


Why dont we want border thin for vertical?

both vertical and horizontal has $ouiBorderThin which set on line 125.

Oops yeah my bad

ashwin-pc · 2023-05-10T15:55:37Z

src/components/button/button_group/_button_group_button.scss

@@ -123,20 +123,43 @@
 .ouiButtonGroup--small {
  .ouiButtonGroupButton {
    border: $ouiBorderThin;
+  }


Suggested change

}

&:not(:first-child) {

margin-left: -1px;

}

}

This is set on line 133 under &:not(.ouiButtonGroup--vertical)

I should not review PR's when im sleepy. yes you are right. I did not notice that the margin properties changed are different even though the values are the same.

KrooshalUX · 2023-05-11T05:12:49Z

@ruanyl great question re: compressed. I'm not entirely familiar with the implementation details so apologies if this is a bit naïve - Is there an approach where we can expand compressed beyond just a boolean? So if orientation = vertical and compressed = true then fixed width and if orientation = horizontal and compressed = true then fixed height ?

I'm wondering if the way this was structured is why some of the other components have orientation split into different components all together.

ruanyl · 2023-05-11T09:54:11Z

@KrooshalUX

So if orientation = vertical and compressed = true then fixed width and if orientation = horizontal and compressed = true then fixed height

Technically it is doable, but if orientation = vertical with fixed width could potentially problematic as the button text might overflow.

I'm wondering if the way this was structured is why some of the other components have orientation split into different components all together.

Do you have an example of this?

BSFishy · 2023-05-11T16:03:45Z

Do you have an example of this?

https://oui.opensearch.org/1.1/#/navigation/steps

AMoo-Miki · 2023-06-21T16:14:03Z

src/components/button/button_group/button_group.tsx

@@ -166,13 +171,24 @@ export const OuiButtonGroup: FunctionComponent<Props> = ({
    );
  }

+  // Compressed style can't support `vertical` orientation because compressed button group has a fixed height


I don't get why this would be a blocker. A variation of height: auto with a fixed-width would be enough to bypass this. no?

@KrooshalUX what's your opinion of a vertical compressed button group should look like?

@KrooshalUX It would be nice to have UX suggestion on a vertical + compressed button

Docs on compressed: https://oui.opensearch.org/1.2/#/forms/compressed-forms

I think the idea is to have any sort of spacing smaller than uncompressed

joshuarrrr · 2023-08-28T22:50:26Z

@ruanyl @KrooshalUX Is this a requirement for any 2.10 features?

ruanyl · 2023-08-28T23:29:37Z

@joshuarrrr No, this is just an improvement

KrooshalUX · 2023-08-28T23:34:02Z

@joshuarrrr nope, just something we want eventually.

@ruanyl can you outline where we are blocked ?

ruanyl · 2023-08-29T00:15:04Z

@KrooshalUX This is not a blocker.

KrooshalUX · 2023-08-29T00:16:32Z

@ruanyl oh, no , to clarify - understood its not a blocker for 2.10, but where are we blocked on finishing the issue? I am not sure if there is any outstanding decisions / need from UX ?

joshuarrrr · 2023-09-08T17:12:35Z

I'm going to mark this as a draft for now, but @ruanyl feel free to take it out of draft status when you're able to make the updates.

This commit added a new prop(orientation) to the current OuiButtonGroup, the possible values are `horizontal` and `vertical`. The default value is `horizontal` which match the current UI behavior. With `vertical` prop, the button group will be oriented vertically. related issue: opensearch-project#659 Signed-off-by: Yulong Ruan <[email protected]>

ruanyl · 2024-10-30T07:37:19Z

@virajsanghvi @AMoo-Miki @ashwin-pc Finally get the PR updated, could you take another look please?

virajsanghvi · 2024-10-30T17:33:13Z

Can you add a changelog entry?

Curious, where would this be used in OSD? Is this part of any designs?

ruanyl · 2024-10-31T01:38:20Z

Can you add a changelog entry?

Curious, where would this be used in OSD? Is this part of any designs?

Sure, I will update the changelog.

This change was originally introduced for maps plugin, it currently uses two buttons vertically aligned, but would be better to use a button group.

Signed-off-by: Yulong Ruan <[email protected]>

ruanyl · 2024-11-04T02:55:07Z

@virajsanghvi PR updated, could you take another look please?

Signed-off-by: Yulong Ruan <[email protected]>

* feat: vertical oriented button group This commit added a new prop(orientation) to the current OuiButtonGroup, the possible values are `horizontal` and `vertical`. The default value is `horizontal` which match the current UI behavior. With `vertical` prop, the button group will be oriented vertically. related issue: #659 Signed-off-by: Yulong Ruan <[email protected]> * doc(changelog): add changelog for vertical button group Signed-off-by: Yulong Ruan <[email protected]> * fix(lint): fix sass lint Signed-off-by: Yulong Ruan <[email protected]> --------- Signed-off-by: Yulong Ruan <[email protected]> (cherry picked from commit 3c191a5) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> # Conflicts: # CHANGELOG.md

ruanyl requested review from seanneumann, AMoo-Miki, rednaksi91, KrooshalUX, BSFishy and bandinib-amzn as code owners May 10, 2023 04:44

github-actions bot added the first-time-contributor label May 10, 2023

ashwin-pc reviewed May 10, 2023

View reviewed changes

AMoo-Miki reviewed Jun 21, 2023

View reviewed changes

joshuarrrr marked this pull request as draft September 8, 2023 17:12

joshuarrrr mentioned this pull request Oct 30, 2023

[Feature Request] Vertical button group #659

Open

ruanyl force-pushed the button-group-vertical branch from 0f0536a to 5c00252 Compare October 30, 2024 07:34

ruanyl marked this pull request as ready for review October 30, 2024 07:36

ruanyl requested review from joshuarrrr and virajsanghvi as code owners October 30, 2024 07:36

ruanyl added 2 commits October 31, 2024 09:39

doc(changelog): add changelog for vertical button group

06f9510

Signed-off-by: Yulong Ruan <[email protected]>

fix(lint): fix sass lint

8b17893

Signed-off-by: Yulong Ruan <[email protected]>

Merge branch 'main' into button-group-vertical

a84a52c

Signed-off-by: Yulong Ruan <[email protected]>

Merge branch 'main' into button-group-vertical

9fa21c5

Signed-off-by: Yulong Ruan <[email protected]>

virajsanghvi approved these changes Nov 13, 2024

View reviewed changes

virajsanghvi added the backport 1.x label Nov 13, 2024

virajsanghvi merged commit 3c191a5 into opensearch-project:main Nov 13, 2024
16 checks passed

opensearch-trigger-bot bot mentioned this pull request Nov 13, 2024

[Backport 1.x] feat: vertical oriented button group #1461

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: vertical oriented button group #755

feat: vertical oriented button group #755

ruanyl commented May 10, 2023 •

edited

Loading

ashwin-pc left a comment

ashwin-pc May 10, 2023

ruanyl May 11, 2023

ashwin-pc May 11, 2023

ashwin-pc May 10, 2023

ruanyl May 11, 2023

ashwin-pc May 11, 2023

KrooshalUX commented May 11, 2023

ruanyl commented May 11, 2023

BSFishy commented May 11, 2023

AMoo-Miki Jun 21, 2023

ruanyl Jun 22, 2023 •

edited

Loading

ruanyl Aug 29, 2023

BSFishy Sep 8, 2023

joshuarrrr commented Aug 28, 2023

ruanyl commented Aug 28, 2023

KrooshalUX commented Aug 28, 2023

ruanyl commented Aug 29, 2023

KrooshalUX commented Aug 29, 2023

joshuarrrr commented Sep 8, 2023

ruanyl commented Oct 30, 2024

virajsanghvi commented Oct 30, 2024

ruanyl commented Oct 31, 2024

ruanyl commented Nov 4, 2024

-  }
+    &:not(:first-child) {
+      margin-left: -1px;
+    }
+  }

feat: vertical oriented button group #755

feat: vertical oriented button group #755

Conversation

ruanyl commented May 10, 2023 • edited Loading

Description

Issues Resolved

Check List

ashwin-pc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KrooshalUX commented May 11, 2023

ruanyl commented May 11, 2023

BSFishy commented May 11, 2023

Choose a reason for hiding this comment

ruanyl Jun 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshuarrrr commented Aug 28, 2023

ruanyl commented Aug 28, 2023

KrooshalUX commented Aug 28, 2023

ruanyl commented Aug 29, 2023

KrooshalUX commented Aug 29, 2023

joshuarrrr commented Sep 8, 2023

ruanyl commented Oct 30, 2024

virajsanghvi commented Oct 30, 2024

ruanyl commented Oct 31, 2024

ruanyl commented Nov 4, 2024

ruanyl commented May 10, 2023 •

edited

Loading

ruanyl Jun 22, 2023 •

edited

Loading