Skip to content
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

fix(FeedbackDialog): update colors and animations #977

Merged
merged 24 commits into from
Sep 15, 2020

Conversation

jnm2377
Copy link
Contributor

@jnm2377 jnm2377 commented Aug 25, 2020

Closes #790
Closes #886

Changelog

  • changes Feedback launch button to use same colors as BackToTop button.
  • changes submitted checkmark animation according to new specs (animate the circle rather than the checkmark)
  • changes the animation for FeedbackDialog:
    • no longer "fade in" type of animation
    • animates width and height, with width animation being faster/finishing before height
    • button row in feedback dialog is also animated in now (height expansion, too) but with a slight delay (added a delayed state change for this functionality animateButtonRow)

Removed

  • no longer using CSSTransition for the animations

@jnm2377 jnm2377 requested review from jeanservaas, a team, vpicone and sstrubberg and removed request for a team August 25, 2020 15:28
@vercel
Copy link

vercel bot commented Aug 25, 2020

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/carbon-design-system/gatsby-theme-carbon/41x9vwjti
✅ Preview: https://gatsby-theme-carbon-git-fork-jnm2377-fix-feeback-btn.carbon-design-system.vercel.app

@vercel vercel bot temporarily deployed to Preview August 25, 2020 15:28 Inactive
@vpicone
Copy link
Collaborator

vpicone commented Aug 25, 2020

Wicked! Are the buttons supposed to come in at the end? I think things are coming in a little dishevled for the entrance, you can slow it down in the chrome animation tools to help debug it:
Screen Recording 2020-08-25 at 11 43 41 AM

@jnm2377
Copy link
Contributor Author

jnm2377 commented Aug 25, 2020

@vpicone yes to the buttons coming in at the end. Ughhh to the disheveled part. I thought I fixed that. It seems like it happens randomly, then. Need to figure it out... 👀

@vpicone
Copy link
Collaborator

vpicone commented Aug 25, 2020

@jnm2377 The checkmark is gonna be so dope!! At the moment, it gets to about 75% and then snaps to the finish. I wasn't able to get a good gif without pulling the code down an forcing the dialog to stay open but let me know if that tracks.

@jnm2377
Copy link
Contributor Author

jnm2377 commented Aug 25, 2020

@vpicone here's what I'm seeing when I slow down the animation.
Still trying to figure out how not to let the content wrap, but idk why the button animations are different for you. Seems like its not consistent in that case, which isn't good either. Any thoughts on what might be making it animate in like that?
feedback

Also, I think the circle animation appears like it snaps to finish bc of the fill. The circle stroke itself transitions until 99%, and then at 100% I add the bg fill. But I think bc it's happening so fast, it looks like it finishes at 75%.

@vpicone
Copy link
Collaborator

vpicone commented Aug 25, 2020

@jnm2377 huh that's so weird. Is that recording from your local build or the build preview? It seems like they're waiting the 400ms before starting but not sure how to replicate it. I checked ff, chrome, and safari with the build preview and they're coming in delayed.

It looks very cyborgy. Maybe this is a feature and not a bug haha

@jnm2377
Copy link
Contributor Author

jnm2377 commented Aug 25, 2020

@vpicone the buttons are supposed to be coming in delayed. But expanding from the bottom up, where as your screen recording showed them coming in from the middle then moving down...which is not the intention. That was my local build. Also, I updated the checkmark animation. Lmk if that looks better.

@vpicone
Copy link
Collaborator

vpicone commented Aug 25, 2020

@jnm2377 okay cool, the expansion from the middle might just be a side effect of the chrome dev tool impacting css/js time differently

@shixiedesign
Copy link

shixiedesign commented Aug 26, 2020

Hey always happy to see motion getting build! I know it's tricky. Here are the main issues I noticed in order of priority:

  • Exit animation of the modal looks not right. I noticed my prototype I didn't include exist. It should first shrink in height, then shrink in width. Currently it's the opposite. If this is too hard to do, consider only shrinking the height. Width change should be secondary & subtle. When the primary motion is width shrinking it feels like a drawer pulling out to the right, which feels very unexpected.

  • Buttons sliding up looks a tad tooooo delayed, which is making it feel stuck. The delay should be very small like 10ms-15ms (see screenshot of Principle timeline below, btn starts coming in 12ms later than Group)

image

@johnbister
Copy link

@jnm2377 it looks good to me!
feebackdialog

@jnm2377 jnm2377 merged commit e55b672 into carbon-design-system:main Sep 15, 2020
@jeanservaas
Copy link
Contributor

@vpicone @mjabbink I think we need to revert this. Sorry, I meant for John to offer some suggestions. I feel that the motion/timing is still a little awkward... even though technically it's following our motion guidance. I think that left then up expansion could be a little bit more subtle.

@jnm2377
Copy link
Contributor Author

jnm2377 commented Sep 15, 2020

@jeanservaas if by left and up expansion, you mean width first then height, this is doing exactly that. You can see it in the console screenshot that John posted. The width finishes transitioning before the height.

@jnm2377
Copy link
Contributor Author

jnm2377 commented Sep 15, 2020

@jeanservaas we could make the width transition slower or faster, but we can't stagger the transitioning start time because the transitioned properties are on the same element, so the transition will be executed at the same time. As well as the fact that if there is no visible height, you won't notice the width transition at all, so they have to start together.

@vpicone
Copy link
Collaborator

vpicone commented Sep 15, 2020

@jnm2377 we might be able to move them independently with key frames I think


@keyframes whatever {
  0% {
    margin-left: 100%;
    width: 300%; 
  }

  25% {
    margin-left: 90%;
  }

  75% {
    font-size: 300%;
    margin-left: 25%;
    width: 150%;
  }

  100% {
    margin-left: 0%;
    width: 100%;
  }
`

@jnm2377
Copy link
Contributor Author

jnm2377 commented Sep 15, 2020

@vpicone but if you have no visible height, you won't see the width transition. Does that make sense? That's what was throwing me off. Because even in the video example that Shixie created, they start transitioning at the same time, but the width just finishes way before the height does.

@jnm2377
Copy link
Contributor Author

jnm2377 commented Sep 15, 2020

@vpicone I can open up a PR to try to time it better in keyframes, but they would still have to start at the same time in order for any of it to be visible.

@jeanservaas
Copy link
Contributor

@jnm2377 yeah totally understand that it's technically following our guidance — but @vpicone and I went through the same thing when we were working on it initially. Vince seemingly followed the guidance and we just couldn't really make it work... Growing the width and then the height felt clunky and we couldn't smooth it out.

Ultimately, we just threw it out and we agreed that Vince would basically just copy the motion on the Gatsby site's feedback component. Shixie gave us some advice for the motion and I added it to the issue thinking that maybe it would shed some light on how to make this work correctly.

I think before moving forward with this, just like John's PRs, we're going to need a review from one of the system's motion experts. @shixiedesign is supposed to weigh in on this tonight or tomorrow. If she can't get to it I will try to have Wonil or even Pete Garvin review it.

@mjabbink
Copy link
Contributor

Agree it needs a bit more work

@shixiedesign
Copy link

shixiedesign commented Sep 16, 2020

Hey everyone. It actually looked much better from the last time I saw it but I'd agree with not merging. It's still calling too much attention to itself.

  1. firstly let's lock / freeze what's NOT supposed to move. Everything inside the module minus the buttons shouldn't move relative to itself. Think of it as an image inside the parent container that gets masked when the parent shrinks/grows. Right now we catch moments like this where the little emojis are all squished

image

  1. I see the width-then-height motion is there in the entrance. Well done! It's probably clunky because it is taking too long. In my prototype, the entire module reaches full width and height by 240ms. It took 400ms in the build.

  2. Buttons are still not the last to finish. The buttons should arrive at final position after module reaches full height.
    image

  3. 👍 closing animation is much better than before. It lacks the width-then-height thing but it's okay in this case. Much more efficient.

Here's a box link to the video prototype. I improved it a bit (color and added placeholder box for "content") but motion is unchanged. https://ibm.box.com/s/2o0eacmb4knebvpnba5vjldzk4sjhb0r

@mjabbink
Copy link
Contributor

I think this works better @shixiedesign.

@johnbister
Copy link

johnbister commented Sep 16, 2020

dialogmockup

I was able to tweak the keyframes you had and get it pretty close to Shixie's prototype. @jnm2377 Let me know if this is helpful. I'm available today if you want to sync up.

@mjabbink
Copy link
Contributor

@shixiedesign @johnbister Maybe a bit slow?

@mjabbink
Copy link
Contributor

Is closing a hair too fast? Maybe fine @shixiedesign @johnbister

@mjabbink
Copy link
Contributor

@jeanservaas @jnm2377 The colors in preview are not right. I think on active state it should remain as a hover and not be dark and a solid face. @jeanservaas can you revisit this and provide spec.

@mjabbink
Copy link
Contributor

mjabbink commented Sep 16, 2020

There is a lot of color/hover activity for such a simple thing.

———
Screen Shot 2020-09-16 at 9 56 54 AM

Screen Shot 2020-09-16 at 9 53 41 AM

Screen Shot 2020-09-16 at 9 53 49 AM

@jeanservaas
Copy link
Contributor

@jnm2377

Enabled state should look like this:

image

Both hover and selected state should look like this:

image

@mjabbink
Copy link
Contributor

@jnm2377 Seems more simple. @jeanservaas whatta ya think?

@vpicone
Copy link
Collaborator

vpicone commented Sep 16, 2020

@jeanservaas @mjabbink reverting this PR, please make an issue with a proper motion spec.

@johnbister
Copy link

@shixiedesign @johnbister Maybe a bit slow?

@mjabbink I sped it up a bit. Let me know if you think this works as a compromise. Any faster and I have a hard time seeing any easing out.
dialog_faster.mp4.zip

johnbister added a commit to johnbister/gatsby-theme-carbon that referenced this pull request Nov 5, 2020
johnbister added a commit to johnbister/gatsby-theme-carbon that referenced this pull request Nov 5, 2020
johnbister added a commit to johnbister/gatsby-theme-carbon that referenced this pull request Nov 5, 2020
johnbister added a commit to johnbister/gatsby-theme-carbon that referenced this pull request Nov 5, 2020
johnbister added a commit to johnbister/gatsby-theme-carbon that referenced this pull request Nov 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feedback button: Make them the same gray color. feedback component: update motion per shixie's specs
7 participants