-
-
Notifications
You must be signed in to change notification settings - Fork 1
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
Add button and drag to swap colors #227
Conversation
✅ Deploy Preview for oddcontrast ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
* main: lint Bump the dev-minor group across 1 directory with 15 updates Bump jsdom from 25.0.1 to 26.0.0 in the dev-major group Bump the dev-minor group with 6 updates upgrade node and yarn Bump the dev-minor group with 6 updates Bump the dev-minor group with 7 updates Bump the dev-minor group with 12 updates
Functionality looks good. I think the button could use some more styling, especially around interactions – since it takes up a lot of space, but only has a small icon. @dvdherron is this something you'd be interested in looking at? |
* main: upgrade stylelint Bump the dev-minor group with 11 updates upgrade all deps Bump the dev-major group with 3 updates
@mirisuzanne I decreased the size of the button target area and moved the button to be more centered in f2b0be1 It also rotates when the colors are stacked Played around with some possible animations but couldn't come up with something that made sense in both orientations. Open to suggestions if you think an animation of some sort should happen. Maybe it just spins on hover or when the colors change? |
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.
Looking pretty good. A swap rotation animation could be fun, but what I was really trying to get at was something more noticeable on hover/focus - to make the click target more obvious. Looking closer, I wonder why we're removing focus rings on all icon buttons currently?
|
||
<button type="button" onclick={switchColors} data-btn="icon switch"> | ||
<Icon name="switch" /> | ||
<span class="sr-only">Click to swap colors</span></button |
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.
<span class="sr-only">Click to swap colors</span></button | |
<span class="sr-only">Swap colors</span></button |
src/sass/initial/_layout.scss
Outdated
'bginput switch fginput' auto | ||
'bgslide . fgslide' auto / 1fr min-content 1fr; |
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.
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
I agree that a rotate-180 rotation animation would be cool here, but doesn't need to hold up merge if we don't want to mess with it. |
I think we gave the icon buttons custom focus styles in b62fd12 ? @stacyk Might have more to say on that. They generally scale up and change color on keyboard focus. I added an outline to swap-colors button for now but we can remove that if we want all the icon buttons to match. |
Added a rotation animation to the button in 3a699a0 And there's now a thin outline on keyboard focus. Should be ready for a final review. |
I like the animation! A couple questions-
|
src/sass/patterns/_buttons.scss
Outdated
&:focus { | ||
--outline-width: thin; | ||
|
||
animation: swap var(--fast) var(--out-back); |
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.
The transform
property already has a transition on it, so might be simpler to rotate rather than add a full animation:
animation: swap var(--fast) var(--out-back); | |
transform: rotate(180deg); |
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.
removed the full animation in ad9a6f9
Good call. I'll add a
Yeah, that might make more sense for the animation to happen as the swap is happening. Would that be adding a temporary class to the button? @jamesnw I can clean up the css if you set up the JS. |
@dvdherron @jamesnw - while I see the logic to it, I don't think that animating on active would be as helpful. At that point we've already taken the action. By doing it on hover, we tell you something about what the action is going to be. |
Instead of rotating, it just scales a tiny bit for people with |
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.
looks great!
Description
Currently not designed, this is to show possibilities.
Related Issue(s)
#108
Todo