TBD Proposal cards animation #4996
Conversation
| {#each $openSnsProposalsStore as proposalInfo (proposalInfo.id)} | ||
| <NnsProposalCard {proposalInfo} /> | ||
| {#each $openSnsProposalsStore as proposalInfo, index (proposalInfo.id)} | ||
| <NnsProposalCard {proposalInfo} {index} /> |
There was a problem hiding this comment.
Do we really want the proposal on the SNS page to slide in while the other 2 cards don't?
|
|
||
| export let universe: Universe; | ||
| export let proposals: ProposalData[]; | ||
| export let cardIndex = 0; |
There was a problem hiding this comment.
How about cardIndexOffset?
|
|
||
| export let universe: Universe; | ||
| export let delay = 0; | ||
| export let noAnimation = false; |
There was a problem hiding this comment.
Do we need this prop? Just leaving the delay at 0 has the same effect, no?
|
|
||
| export let universe: Universe; | ||
| export let delay = 0; |
There was a problem hiding this comment.
Let's use a clearer name. Something like titleFadeInDelay?
| import { isNode } from "$lib/utils/dev.utils"; | ||
| import { isHtmlElementInViewport } from "$lib/utils/utils"; | ||
|
|
||
| export let index = 0; |
There was a problem hiding this comment.
Can we use a more specific name? Something like animationIndex?
Same for similar props in other card components.
| {#each actionableUniverses as { universe, proposals } (universe.canisterId)} | ||
| <ActionableSnsProposals {universe} {proposals} /> | ||
| <ActionableSnsProposals {universe} {proposals} {cardIndex} /> |
There was a problem hiding this comment.
Shouldn't cardIndex be incremented per each universe by the number of cards before it?
|
|
||
| let inViewport: boolean = false; | ||
| $: inViewport = | ||
| isNode() || isNullish(element) ? true : isHtmlElementInViewport(element); |
There was a problem hiding this comment.
Why exactly do we call isNode here from dev.utils?
|
|
||
| let inViewport: boolean = false; | ||
| $: inViewport = | ||
| isNode() || isNullish(element) ? true : isHtmlElementInViewport(element); |
There was a problem hiding this comment.
If I read the logic correctly, we treat an element that is null is being in the viewport. Is that correct?
| top >= 0 && | ||
| left >= 0 && | ||
| bottom <= window.innerHeight && | ||
| right <= window.innerWidth |
There was a problem hiding this comment.
This is only true if the element is entirely inside the viewport. If even the slightest bit is outside but the element is mostly visible, it will return false.
Shouldn't it be the opposite where we want to animate a card also if it's partially visible?
Either way, we should clearly document this.
| export const SVELTE_DEFAULT_ANIMATION_DURATION_IN_MILLISECOND = 400; | ||
| export const PROPOSAL_CARD_ANIMATION_DURATION_IN_MILLISECOND = | ||
| SVELTE_DEFAULT_ANIMATION_DURATION_IN_MILLISECOND; | ||
| // Maximum 10 card animations at a time |
There was a problem hiding this comment.
What does this comment refer to?
It doesn't seem to have anything to do with the line above or below it.
And should it be 18 instead of 10? The PR description says 18. Or is that something else?
Motivation
This PR adds animation to the /proposals page cards, making it clearer for users to understand that the universe were switched. The animation will also keep users engaged and improve the overall user experience.
Changes
isHtmlElementInViewport.ProposalCardcomponent, allowing each card to determine its animation start time (with a default value, since it's not a critical feature).ProposalCardcomponent:|globalmodificator was applied to make the animation work between pages. Onlyintransition was used. No navigation issues detected.UniverseWithActionableProposalsto prevent animation of the first headline and initiate animation of the next headline after the standard animation duration.Tests
Todos