Skip to content

ActivityItem: add pulsing beacon animation#4553

Merged
lynamemi merged 31 commits intomicrosoft:masterfrom
lynamemi:activity-item-animation
Apr 18, 2018
Merged

ActivityItem: add pulsing beacon animation#4553
lynamemi merged 31 commits intomicrosoft:masterfrom
lynamemi:activity-item-animation

Conversation

@lynamemi
Copy link
Copy Markdown
Collaborator

Pull request checklist

  • Addresses an existing issue: Fixes #0000
  • Include a change request file using $ npm run change

Description of changes

  • Using PulsingBeaconAnimationStyles from the styling package for the pulse. This code is shared with Coachmark, which uses a double pulse. ActivityItem needs a single pulse so I had to adjust the interface.
  • New prop to trigger the animation: animatePulsingBeacon.
  • With the pulse, the whole item fades in and the text slides in.
  • Added a new snapshot test for the animation styles.
  • Adjusted some of the icon styling so that the pulse will center on Personas and icons alike.
  • Will be used in ItemsView's Activity Column.

Focus areas to test

(optional)

lynamemi added 21 commits April 2, 2018 14:09
@lynamemi lynamemi requested a review from phkuo as a code owner April 13, 2018 22:15
animationName: continuousPulse,
animationIterationCount: '1',
animationDuration: '.8s',
zIndex: 1000
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

is there a way to do this without z-index? z-index can be deceptive unless you can guarantee your own stacking context

Copy link
Copy Markdown
Collaborator Author

@lynamemi lynamemi Apr 16, 2018

Choose a reason for hiding this comment

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

I think I do need some z-index during the animation so that it animates over the icon / persona and the text. But it's only for within the Item itself so I lowered it. - I'll push that change once the master build is fixed.


export class ActivityItem extends BaseComponent<IActivityItemProps, {}> {
public static defaultProps: Partial<IActivityItemProps> = {
beaconColorOne: '#00FFEC',
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

do we need themable colors for this or is it just a shine?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

verify this looks okay in a dark theme

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I got the colors from Coachmark - confirming with design what to do here.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

We switched to comparable theme colors. @phkuo how do you suggest testing dark theme? Is there a SP palette we can plug-in to the demo site?

height: 36,
beaconColorOne: '#00FFEC',
beaconColorTwo: '#005EDD',
beaconColorOne: DefaultPalette.blueLight,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

use one of the theme* colors, since blueLight won't really change theme-to-theme (and will be very different than the themePrimary below in many themes)

@lynamemi
Copy link
Copy Markdown
Collaborator Author

@phkuo The pulsing beacon now works in dark theme! Thanks for your help in testing that. I got rid of 'static default props' for beaconColorOne /Two and am handling the default fallback instead in styles.ts to get access to the theme object.

animationName: continuousPulse,
animationIterationCount: '1',
animationDuration: '.8s',
zIndex: 1000
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

if this is within its own stacking context, then just set z-index to 2

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

(because no other z-indexes elsewhere will matter)

Copy link
Copy Markdown
Contributor

@phkuo phkuo left a comment

Choose a reason for hiding this comment

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

see comment

@lynamemi lynamemi merged commit 7d06629 into microsoft:master Apr 18, 2018
@lynamemi lynamemi deleted the activity-item-animation branch April 18, 2018 20:19
@microsoft microsoft locked as resolved and limited conversation to collaborators Aug 31, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants