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

feat(button): sample animation on button and alert group #6542

Draft
wants to merge 1 commit into
base: v6
Choose a base branch
from

Conversation

srambach
Copy link
Member

@srambach srambach commented Apr 16, 2024

As part of #6515 spike, this shows a few things:

  • motion tokens added. These exist in Figma but were copied over manually for now
  • also adds and uses a test duration of 1s so you can see what's going on
  • Button has a transition so on hover you can see the color slowly change (1s). This transition has no reduced motion alternative.
  • Alert group item has a new class .pf-m-remove. This class causes the item to be removed by sliding off to the right (translateX), or for reduced-motion, it fades (opacity).
  • All transitions are removed when the class .pf-m-no-motion is applied to a parent. (no matter what the reduced motion setting is) This is intended to be used for testing purposes.

To test:

@patternfly-build
Copy link

patternfly-build commented Apr 16, 2024

@srambach srambach linked an issue Apr 16, 2024 that may be closed by this pull request
@andrew-ronaldson
Copy link
Collaborator

Screen.Recording.2024-04-17.at.2.55.58.PM.mov

Copy link
Contributor

@thatblindgeye thatblindgeye left a comment

Choose a reason for hiding this comment

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

This is looking pretty nice ✨

Some questions/things I noticed, which if it isn't applicable right now/if any of this is already known feel free to ignore:

  • Noticing for the Alert, the no-motion modifier class doesn't cause the fade in/out effect that enabling the "prefers reduced motion" setting in the OS.
  • For buttons, is it intended that borders would have the same effect on hover as backgrounds? Secondary and tertiary don't look to have any sort of effect on hover (other than the abrupt change in color)
  • is the no-motion class intended purely for testing on our part, or do we intend for consumers to be able to utilize it as well so that users could manually enable/disable motion outside of an OS setting?

@srambach
Copy link
Member Author

srambach commented May 14, 2024

@thatblindgeye I'm finally getting around to revisiting this!

  • Noticing for the Alert, the no-motion modifier class doesn't cause the fade in/out effect that enabling the "prefers reduced motion" setting in the OS.

This is by design - that modifier is intended to kill all motion/animation/transitions so that automated testing won't be confused. In contrast, yes, prefers-reduced-motion might still have some things happening like a fade instead of a slide.

  • For buttons, is it intended that borders would have the same effect on hover as backgrounds? Secondary and tertiary don't look to have any sort of effect on hover (other than the abrupt change in color)

There wasn't a final design - this was just noodling around.

  • is the no-motion class intended purely for testing on our part, or do we intend for consumers to be able to utilize it as well so that users could manually enable/disable motion outside of an OS setting?

TBD? Maybe it's useful for their automated testing but probably not as a user setting.

Comment on lines +16 to +46
// stylelint-disable
.pf-m-no-motion * {
// simplest way of turning off all animation
// disadvantage: takes the wildcard selector
// transition: none !important;
// animation: none !important;
}
// styleline-enable

.pf-m-no-motion {
// instead we can set all (or just semantic?) motion tokens to 0
// disadvantage - need to catch any new tokens created
// note - relies on always using tokens
--pf-t--global--delay--400: 0s;
--pf-t--global--delay--300: 0s;
--pf-t--global--delay--200: 0s;
--pf-t--global--delay--100: 0s;
--pf-t--global--duration--300: 0s;
--pf-t--global--duration--200: 0s;
--pf-t--global--duration--100: 0s;
--pf-t--global--duration--000: 0s;
--pf-t--global--motion--delay--long: 0s;
--pf-t--global--motion--delay--default: 0s;
--pf-t--global--motion--delay--short: 0s;
--pf-t--global--motion--delay--none: 0s;
--pf-t--global--motion--duration--long: 0s;
--pf-t--global--motion--duration--default: 0s;
--pf-t--global--motion--duration--short: 0s;
// added just for testing purposes
--pf-t--global--motion--duration--test: 0s;
}
Copy link
Member Author

Choose a reason for hiding this comment

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

@mattnolting @mcoker I'd be interested in your thoughts about the two methods of turning off all motion here. (this isn't reduced motion - it's turning everything off for automated testing purposes)

Copy link
Contributor

Choose a reason for hiding this comment

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

In #6369, Garrett notes performance impacts of using global selectors. At the least, I'd say > * is nonviable. Second approach 👍

You can see it uses > * in a few places (as well as :first-child, which isn't nearly as bad, but still not ideal). As it's children of the tr, those can only be th and td according to tr's content model (except for non-visible elements like script and template, which CSS doesn't need to touch). Even :where(th, td) should be much better. This is because CSS matches from the right to the left, so those lines are saying "look at every element on the page (including children and parents of the wanted elements as well as everything else), then see if it has a parent of a th" instead of first narrowing it down to td and th elements.

@mcoker
Copy link
Contributor

mcoker commented May 31, 2024

@srambach do you want to keep this open or is it ok to close?

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.

SPIKE Animation - revisit and validate approach
6 participants