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

Chore/utility-bar-icons-animation #961

Merged
merged 23 commits into from
Jul 12, 2022
Merged

Conversation

Huongg
Copy link
Contributor

@Huongg Huongg commented Jul 5, 2022

Description

Fixes #930

Development notes

ezgif com-gif-maker (4)

QA notes

  • I currently apply 300 ms as a duration for the fade-in/out effect, does it look okay? Any suggestion @GabrielComymQB @Mackay031 ?

  • Also, I currently put sliding-animation.js in the same location as experiment-primary-toolbar.js, I agree this is not the right place but I'm not 100% sure where would best the best place for this? Let me know if you think it should belong somewhere else @tynandebold @rashidakanchwala @cvetankanechevska

  • I also use react-transition-group which is also installed for kedro-viz but have not been used anywhere. I personally like to use it as it helps to manage all the different states, and if we're thinking of having it as a reusable component somewhere else, it's also possible to implement.

Checklist

  • Read the contributing guidelines
  • Opened this PR as a 'Draft Pull Request' if it is work-in-progress
  • Updated the documentation to reflect the code changes
  • Added new entries to the RELEASE.md file
  • Added tests to cover my changes

@comym
Copy link

comym commented Jul 6, 2022

I like it, looking good to me
I just have some small comments:

1 - I'm not sure if you're using any ease to the animation. You can try applying some easy in/out to break out the stiffness of the movement. Maybe you're are already using it and it's just very subtle.

2 - Timewise I think you can even reduce the time of 300ms, just a touch. Feel free to experiment and see what feels right.

3 - Another thing I noticed is that the icons come in an inactive state and only when the animation stops, they come active/lit.
Is this happening for some technical reason? Cause if not it would be great - if the icon is available/active - that it could come in its original state straight away. So we would avoid this kind of "turning on blink" at the end of the animation.

Thanks

@Huongg Huongg requested a review from yetudada as a code owner July 6, 2022 08:41
Copy link
Contributor

@rashidakanchwala rashidakanchwala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. thanks @Huongg

i also noticed the icons are in disabled state in the beginning and then active but I wouldn't have noticed if Gabriel hadn't commented it. It happens very fast :)

@Huongg
Copy link
Contributor Author

Huongg commented Jul 6, 2022

thanks @GabrielComymQB and @rashidakanchwala for taking a look, for the last point I think it comes from the opacity changing from 0 -> 1 when it's sliding in, so it might look like it was inactive => active again but it's just the opacity changing, let me think what the alternative changes for that

@cvetankanechevska
Copy link
Contributor

Also, noticed this issue with the toolbar for 'IconButton' , I think is something related to your changes in the Sidebar

Screenshot 2022-07-06 at 13 44 49

@Huongg
Copy link
Contributor Author

Huongg commented Jul 6, 2022

Also, noticed this issue with the toolbar for 'IconButton' , I think is something related to your changes in the Sidebar

Screenshot 2022-07-06 at 13 44 49

good spot thank you. This is a tricky part as the tooltip is part of the icon, I can't think of a way at the moment to make it slide underneath the panel on both sides, but at the same time, the tooltip appears on the top. Let me think 🤔

@comym
Copy link

comym commented Jul 6, 2022 via email

@Huongg Huongg force-pushed the chore/utility-bar-icons-animation branch from 360ccc1 to 8d4661d Compare July 7, 2022 15:30
@Huongg Huongg requested a review from cvetankanechevska July 7, 2022 15:43
@Huongg
Copy link
Contributor Author

Huongg commented Jul 7, 2022

hey @GabrielComymQB @tynandebold @cvetankanechevska @rashidakanchwala I just made some changes to address to the tooltip issue, and also to remove the opacity and add in the ease-in-out transition.

Let me know what you all think? I've been staring at this transition all day so I can't really tell the difference anymore :(

@Huongg
Copy link
Contributor Author

Huongg commented Jul 7, 2022

actually, @cvetankanechevska has come up with a way that we don't have to use JS to fix it. It only allows the button to move horizontally around 34% left to right, or right to left but I think it looks better to me, and good thing we dont need to hack it

Copy link
Member

@tynandebold tynandebold left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks, works, and feels solid! Nicely done.

Just one comment from me, though not enough to block merging.

visibility: 'visible',
},
exiting: {
transform: 'translateX(-34%)',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if the 34% value can also be configurable? If we want to use this again, what's the chance we'll want the same percentage? If too much to do right now, just leave as is and we can revisit later.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we can do that by making that value to be dynamically changed. In this case we want to reposition the element horizontally from the start of the Sidebar (left and right) to the center, the Sidebar width is 56px and by 34% is animated to the center from both sides. Maybe we can set this value as we have const duration = 300ms; at the top of the component. What do you think?

@comym
Copy link

comym commented Jul 7, 2022

hey @GabrielComymQB @tynandebold @cvetankanechevska @rashidakanchwala I just made some changes to address to the tooltip issue, and also to remove the opacity and add in the ease-in-out transition.

Let me know what you all think? I've been staring at this transition all day so I can't really tell the difference anymore :(

Hi @Huongg I just checked it here.

I appreciate the effort and I see there are some approvals in here. I can't approve it this way.

I've seen you solved the tooltip issue which is huge, well done. However the animation is still not there.

I see what you're doing and I believe it's almost there. The issue here is because you're are not using masks, as soon as you click the toggle, either way, the icons suddenly appear on screen in their full opacity. It feels a bit jumpy.

For instance, this is the very first frame as soon you toggle it off:
Screenshot 2022-07-07 at 18 21 12

Note that the edges of the icon are coming from outside de utility bar (hence you're not using masks)

My suggestion is that because you're are not masking then, you do use both ease and transparency to achieve a more smooth "appear / disappear" result.

Basically, keep as it is, but when a set of icons are coming IN to the screen, they ramp up from 0% to 100% transparency at the very end of the animation. The same goes for when they move OUT of the screen. The icons in this case would go from 100 to 0% transparency at the end of the animation.

I think we can get to the right result this way. Let me know if you want to jump in a call tomorrow and maybe we can polish it together on the go if you need it.

Many thanks

@Huongg
Copy link
Contributor Author

Huongg commented Jul 7, 2022

hey @GabrielComymQB @tynandebold @cvetankanechevska @rashidakanchwala I just made some changes to address to the tooltip issue, and also to remove the opacity and add in the ease-in-out transition.
Let me know what you all think? I've been staring at this transition all day so I can't really tell the difference anymore :(

Hi @Huongg I just checked it here.

I appreciate the effort and I see there are some approvals in here. I can't approve it this way.

I've seen you solved the tooltip issue which is huge, well done. However the animation is still not there.

I see what you're doing and I believe it's almost there. The issue here is because you're are not using masks, as soon as you click the toggle, either way, the icons suddenly appear on screen in their full opacity. It feels a bit jumpy.

For instance, this is the very first frame as soon you toggle it off: Screenshot 2022-07-07 at 18 21 12

Note that the edges of the icon are coming from outside de utility bar (hence you're not using masks)

My suggestion is that because you're are not masking then, you do use both ease and transparency to achieve a more smooth "appear / disappear" result.

Basically, keep as it is, but when a set of icons are coming IN to the screen, they ramp up from 0% to 100% transparency at the very end of the animation. The same goes for when they move OUT of the screen. The icons in this case would go from 100 to 0% transparency at the end of the animation.

I think we can get to the right result this way. Let me know if you want to jump in a call tomorrow and maybe we can polish it together on the go if you need it.

Many thanks

hey I see what you mean, I've made one small changes hopefully it's getting closer there. yeah lets have a chat tomorrow and get this fixed together 👍

@comym
Copy link

comym commented Jul 8, 2022

Hi team,

I'm unable to check the latest cause Gitpod does not open Viz, not sure what's happening but it happened before. I restarted my laptop and it's still the same

I'm stuck here
image

Thanks

@Huongg Huongg merged commit e688afb into main Jul 12, 2022
@Huongg Huongg deleted the chore/utility-bar-icons-animation branch July 12, 2022 08:47
@tynandebold tynandebold mentioned this pull request Jul 29, 2022
5 tasks
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.

Utility bar icons should animate when changing between ‘single’ and ‘comparison’ modes
5 participants