Sage/DSY-1746 Button Toggle styling changes #6217
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
DobroTora
force-pushed
the
Sage/DSY-1746-Button-Toggle-Minor-Child
branch
from
c6bea56
to
51bb52c
Compare
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit d02e0cc:
|
DobroTora
force-pushed
the
Sage/DSY-1746-Button-Toggle-Minor-Child
branch
2 times, most recently
from
f348b97
to
d87a932
Compare
DobroTora
changed the title
DobroTora
added
Pending Review
Pending QA
Pending UX QA
Pending a11y Review
and removed
Pending a11y Review
labels
DobroTora
force-pushed
the
Sage/DSY-1746-Button-Toggle-Minor-Child
branch
from
e52d4ca
to
4bcc76b
Compare
tomdavies73
reviewed
Jul 28, 2023
src/components/button-toggle/button-toggle-group/button-toggle-group.component.tsx
Outdated
Show resolved
Hide resolved
src/components/button-toggle/button-toggle-group/button-toggle-group.component.tsx
Outdated
Show resolved
Hide resolved
src/components/button-toggle/button-toggle-group/button-toggle-group.stories.tsx
Outdated
Show resolved
Hide resolved
tomdavies73
reviewed
Jul 31, 2023
edleeks87
requested changes
Aug 2, 2023
There was a problem hiding this comment.
We'll need to prevent the content jumping when the minor buttons are pressed/ activated
https://codesandbox.io/s/carbon-quickstart-forked-f82m7p?file=/src/App.js see for example of what I mean
| var(--spacing100); | ||
| } | ||
|
|
||
| &:focus { |
There was a problem hiding this comment.
Comment: you don't need this as far as I can tell. You should style it using the aria-pressed=true attribute similar to how it is achieved above. You'll also need to the padding imo as when the toggle button is active the increased border thickness will cause the layout to jump so the padding will need to be reduced when that happens to avoid it. Something like the below will work:
color: var(--colorsActionMinor500);
border: 1px solid var(--colorsActionMinor500);
&[aria-pressed="true"] {
border: 4px solid var(--colorsActionMinor500);
background-color: transparent;
padding: 0 ${paddingConfig[size] - 3}px;
:hover {
background-color: var(--colorsActionMinor600);
color: var(--colorsUtilityYang100);
border-color: var(--colorsActionMinor600);
}
}
:not([aria-pressed="true"]):not(:disabled):hover {
color: var(--colorsActionMinorYang100);
background-color: var(--colorsActionMinor600);
border-color: var(--colorsActionMinor600);
}
There was a problem hiding this comment.
https://codesandbox.io/s/carbon-quickstart-forked-f82m7p?file=/src/App.js
If you click one of the buttons you will see the content jumping
src/components/button-toggle/button-toggle-group/button-toggle-group.component.tsx
Outdated
Show resolved
Hide resolved
edleeks87
reviewed
Aug 8, 2023
edleeks87
reviewed
Aug 8, 2023
edleeks87
reviewed
Aug 8, 2023
edleeks87
reviewed
Aug 8, 2023
edleeks87
reviewed
Aug 8, 2023
src/components/button-toggle/button-toggle-group/button-toggle-group.spec.tsx
Outdated
Show resolved
Hide resolved
DobroTora
force-pushed
the
Sage/DSY-1746-Button-Toggle-Minor-Child
branch
2 times, most recently
from
db7feb9
to
eaa88d0
Compare
DobroTora
force-pushed
the
Sage/DSY-1746-Button-Toggle-Minor-Child
branch
2 times, most recently
from
4c497ff
to
653f748
Compare
edleeks87
previously approved these changes
Nov 16, 2023
DobroTora
force-pushed
the
Sage/DSY-1746-Button-Toggle-Minor-Child
branch
from
83d6d5b
to
c814b34
Compare
DobroTora
force-pushed
the
Sage/DSY-1746-Button-Toggle-Minor-Child
branch
from
c814b34
to
08d3bc8
Compare
edleeks87
previously approved these changes
Nov 28, 2023
nineteen88
previously approved these changes
Nov 28, 2023
stephenogorman
previously approved these changes
Dec 1, 2023
DobroTora
dismissed stale reviews from stephenogorman, nineteen88, and edleeks87
via
08d3bc8
DobroTora
force-pushed
the
Sage/DSY-1746-Button-Toggle-Minor-Child
branch
from
0e253b6
to
08d3bc8
Compare
feat(button-toggle): rm variant prop feat(button-toggle): rm variant prop, tests feat(button-toggle): fix ts errors feat(button-toggle): update snaps feat(button-toggle): rm variant prop
DobroTora
force-pushed
the
Sage/DSY-1746-Button-Toggle-Minor-Child
branch
from
08d3bc8
to
864f005
Compare
edleeks87
approved these changes
Dec 6, 2023
stephenogorman
approved these changes
Dec 6, 2023
nineteen88
approved these changes
Dec 6, 2023
|
🎉 This PR is included in version 124.2.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Proposed behaviour
New Styling Added to Button Toggle component
Current behaviour
New Styling added to Button Toggle
Checklist
d.tsfile added or updated if requiredQA
Additional context
Testing instructions
The following CodeSandbox is an example of the broken behaviour.
You can see the new behaviour by looking at the version in the comment by
codesandbox[bot].