Conversation
feat: added defeat animations for units and upgrades feat: added animation for shield breaks chore: prep for animations prefs feat: added animations tab feat: added healing animation # Conflicts: # src/app/_components/_sharedcomponents/Cards/GameCard.tsx
mariotee
requested review from
AMMayberry1,
brianries,
danbastin and
moyerr
as code owners
| // (these animations need the elements to still be in DOM) | ||
| if ((hasDefeatAnimations || hasShieldAnimations) && gameState.animations) { | ||
| try { | ||
| await processAnimations(gameState.animations); |
There was a problem hiding this comment.
I like the idea of making this function async and just awaiting the animation instead of trying to juggle pre and post animation game states so thats nice.
I think what might be better here though is to have a property on events that mark them as pre or post update and then disambiguate here. That way the BE can just tell us where in the timeline each event should be rendered. We might also need to tell the manager which animations to process in each window. I'm not sure if we might ahve some events that would require an update before AND after update. Perhaps on a When defeated trigger?
| <Box | ||
| sx={styles.card} | ||
| onClick={handleClick} | ||
| onMouseEnter={handlePreviewOpen} |
There was a problem hiding this comment.
Maybe a bad merge here? I think these were moved down a layer to prevent the preview popup from showing when suing teh distribute controls.
There was a problem hiding this comment.
i see.. and i needed them up a layer due to the defeat animation showing the black box behind it. i'll revert this and then put some styling to make that box transparent
|
Just a couple quick comments here mainly concerning about how we're receiving and processing animation events from the BE. Talekd to Veld and I think we're gonna just try to iron out the event delivery first and then I'll have some time this week to take a closer look at the actual animation part. |
| /** | ||
| * Optimizes the queue by combining duplicate events. | ||
| */ | ||
| private optimizeQueue(): void { |
There was a problem hiding this comment.
Aren't we doing this on the BE already?
| // Damage animations will execute first due to higher priority (90 vs 85) | ||
|
|
||
| const hasDefeatAnimations = gameState.animations?.events?.some( | ||
| (event: any) => event.type === 'DEFEAT' |
There was a problem hiding this comment.
Same note as on the BE, we prefer to use the enum types instead of the string value
| } | ||
| }); | ||
|
|
||
| const [fastAnimations, setFastAnimations] = useState<boolean>(() => { |
There was a problem hiding this comment.
I can't tell if animation speed being a boolean makes this more clean and readable, or a little confusing.
| } | ||
| }; | ||
|
|
||
| element.addEventListener('animationend', handleAnimationEnd, { once: true }); |
There was a problem hiding this comment.
Solid use of vanilla JS stuff in these executors
| } | ||
| } | ||
|
|
||
| /* Top left piece */ |
There was a problem hiding this comment.
The shrapnel shield defeat looks good! I think shrapnel's CSS animation code here makes sense too, so the more intricate animation is worth having imo.
| border-color: rgba(0, 140, 255, 0.4); | ||
| box-shadow: 0 0 4px rgba(0, 140, 255, 0.4); | ||
| } | ||
| `; |
There was a problem hiding this comment.
Are there any shared/global CSS or animations defined in this codebase? I see that this pulseBorder animation's code is duplicated for each settings page at least.
Standardizing and having a source of truth for styles is something that I'd be willing to take on sometime.
| label={ | ||
| <Box> | ||
| <Typography variant="body1" fontWeight="bold">Show Only Damage Animations</Typography> | ||
| <Typography variant="body2" color="text.secondary"> |
There was a problem hiding this comment.
These subtitle Typography components need to have a different color, they're currently defaulting to black and indistinguishable from the background
There was a problem hiding this comment.
color: '#878787' is what the other tabs are using. The theme could be updated to use that color as text.secondary too.
ejmudrak
left a comment
ejmudrak
left a comment
There was a problem hiding this comment.
I'm new to this codebase, but some really exciting changes here. Especially for new players (like myself) that may feel like "wait wtf just happened" when an action is made against one of my units. Good stuff here!
4 participants
feat: added defeat animations for units and upgrades
feat: added animation for shield breaks
chore: prep for animations prefs
feat: added animations tab
feat: added healing animation
Conflicts:
src/app/_components/_sharedcomponents/Cards/GameCard.tsx