-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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(content-switcher): adds fade up animation #10148
feat(content-switcher): adds fade up animation #10148
Conversation
✔️ Deploy Preview for carbon-react-next ready! 🔨 Explore the source changes: e6a224e 🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-react-next/deploys/61fc269b1ffd5c0007d018b5 😎 Browse the preview: https://deploy-preview-10148--carbon-react-next.netlify.app |
DCO Assistant Lite bot All contributors have signed the DCO. |
✔️ Deploy Preview for carbon-elements ready! 🔨 Explore the source changes: e6a224e 🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-elements/deploys/61fc269bbf08ba0008920c21 😎 Browse the preview: https://deploy-preview-10148--carbon-elements.netlify.app |
❌ Deploy Preview for carbon-components-react failed. 🔨 Explore the source changes: e6a224e 🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-components-react/deploys/61fc269b88bb770008dda20d |
I'm getting a 'can't handle controls' error in the storybook, but the motion in the recording looks great to me! Do you see this being a one-off animation for the content switcher or do you see it working for more, similar components (like tab for example)? @mjabbink Do you have any concerns? |
@johnbister I will get back to you after our micro-interaction meeting on thursdays |
@AlexanderMelox can you sign the DCO? just comment on this PR like it says here: #10148 (comment) @johnbister the storybook preview is working for me, here's the link I'm looking at: https://deploy-preview-10148--carbon-components-react.netlify.app/?path=/story/components-contentswitcher--default Maybe you need to clear your cache? 👀 |
I have read the DCO document and I hereby sign the DCO. |
Also, I just noticed the preview does not show the animation, but locally (since we are running |
@jnm2377 I saw that you enabled auto-merge. Can we actually disable it until my final review with my motion team? I don't want to redo another PR to fix something. |
@AlexanderMelox I can disable it. FYI It won't merge anything until it's met all the necessary requirements to merge. Which is 2 dev approvals and 1 design approval, and all tests passing. |
Yes, the styles need to be added in both v10 and v11 packages. @AlexanderMelox |
@johnbister @AlexanderMelox any updates on this? |
Our motion designer is going to do QA on the animation (any last minute tweaks) and adding the styles to the styles package today |
packages/styles/scss/components/content-switcher/_content-switcher.scss
Outdated
Show resolved
Hide resolved
Co-authored-by: Josefina Mancilla <[email protected]>
packages/components/src/components/content-switcher/_content-switcher.scss
Outdated
Show resolved
Hide resolved
@mjabbink I think I just solved the issue! Just pushed so the new preview link should be live soon |
@mjabbink here's the new preview, animations work now |
thanks @AlexanderMelox Have you looked at this animation in context of a UI? I like this but I also wonder if it’s too much for what it is? It certainly get’s a visual emphasis and maybe that’s a good thing in context of all the other UI elements/components. |
@johnbister do you have feedback on the animation? on the dev side the code looks good, but this needs design review/approval before it could be merged. |
@jnm2377 The motion looks great to me, but I have some concerns about how this fits into the larger context of a UI and our other components like I mentioned on 11/26. It seems @mjabbink has some concerns there as well. That being said, I am happy to approve it and let's see how it influences motion in the future. |
Thanks for the reminder @jnm2377 😄 |
We're creating a prototype to showcase the content switcher with animated elements under |
Hmm... @johnbister we don't have to approve the PR if you and Mike still have questions around it. Maybe we should close this PR until @AlexanderMelox designers have some more answers/prototypes for our design team to give feedback on. It seems like the 2 big questions are:
I think these are good discussions to be had in the issue, and if/once we reach an agreement then we can re-open the PR? |
I stand by my decision to approve it. Great work @AlexanderMelox |
Here's a prototype in the context of a UI I don't think it's too much but can add that breathe of fresh air to some stale pages, especially while switching contents. I got the green light from my team to merge this PR ✅ |
@AlexanderMelox Thanks for sending the example--looks cool! I can definitely see how this might influence other components so we'll be careful to see how it plays out. Just a thought for down the road, but this kind of motion would look great for a dark/light theme switcher, too. |
@abbeyhrt when you have the time, can you review this PR so it can be merged? Thanks! |
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.
Sorry for the delay but it looks good to me! Thank you for the contribution!
Looks good @AlexanderMelox |
Closes #10117
Screen.Recording.2021-11-18.at.3.58.29.PM.mov
This PR adds a fade-up micro-interaction to the Content Switcher component. This change only changes how the background appears behind the content switcher. No additional functionality was changed in the component.
Changelog
Changed
Testing / Reviewing
No testing, just visual testing with storybook knobs