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.
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
feat(togglegroup): add toggle group component #3355
Conversation
Preview: https://patternfly-pr-3355.surge.sh A11y report: https://patternfly-pr-3355-coverage.surge.sh CSS Size Report
|
{{/toggle-group-text}} | ||
{{/toggle-group-button}} | ||
{{> divider divider--type="div" divider--modifier="pf-m-vertical"}} | ||
{{#> toggle-group-button toggle-group-button--modifier="pf-m-selected"}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
{{#> toggle-group-button toggle-group-button--modifier="pf-m-selected"}} | |
{{#> toggle-group-button}} |
Looks like you have two set to selected
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is to demonstrated two things can be clicked.
@@ -0,0 +1,6 @@ | |||
<div class="pf-c-toggle-group__icon{{#if toggle-group-icon--modifier}} {{toggle-group-icon--modifier}}{{/if}}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we make this a span?
@@ -0,0 +1,6 @@ | |||
<div class="pf-c-toggle-group__text{{#if toggle-group-text--modifier}} {{toggle-group-text--modifier}}{{/if}}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same, can this be a span?
border-bottom-right-radius: var(--pf-c-toggle-group__button--last-child--BorderBottomRightRadius); | ||
} | ||
|
||
&:hover { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we add a focus state?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added the focus state and going to follow up with @mceledonia on monday
font-size: var(--pf-c-toggle-group__button--FontSize); | ||
color: var(--pf-c-toggle-group__button--Color); | ||
background-color: var(--pf-c-toggle-group__button--BackgroundColor); | ||
border: 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this be an inline-flex
item?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we support icon positioning the same way we do with the button component? That would be another reason to use a button component here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
^ will follow up on this
font-size: var(--pf-c-toggle-group__button--FontSize); | ||
color: var(--pf-c-toggle-group__button--Color); | ||
background-color: var(--pf-c-toggle-group__button--BackgroundColor); | ||
border: 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
border: 0; | |
border: none; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LPTM 🥳
closes #2625
new PR from new forked repo
old PR #3295