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

Logical Animation Directions #422

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

ryanmitchx
Copy link

As per: #390

  • Slide in/out animations have been updated to use {start, end} instead of {left, right}
  • Animations using logical directions autodetect HTML dir attribute
  • Aliases to old prop names have been created
  • Bugfix for --isLTR and --isRTL props
  • Minor build script changes to add in necessary stylesheets and allow 1:n mapping of named animations/keyframes to animation props (vs 1:1 currently)
  • Updated docs with new names

Unsure if this is overkill! Similarly, this seems like a minor version bump with the aliases to the old prop names, but this might break apps for people who've created RTL pages and using the 'opposite' prop to compensate, so I didn't bump the version.

@stackblitz
Copy link

stackblitz bot commented Sep 29, 2023

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

Copy link
Owner

@argyleink argyleink left a comment

Choose a reason for hiding this comment

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

Very cool work so far! The aliases are really nice, almost too nice (see end of this comment!)

All my tests went good, even though they hurt my 20yr trained brain on right and left animations. Wierd because i've switched to logical props for everything else and dont even notice, but these animation directions are different!

There are however, 2 problems, and they're both on me for not communicating good enough in the issue about the requirements:

  1. logical properties / directions have 2 axes, inline and block. you've essentially done inline, but block still remains
  2. the current naming uses start and end without specifying if it's inline or block… so the animation names need to be --animation-slide-out-inline-start and --animation-slide-out-block-end

Kinda means this PR is halfway through the changes needed 😅

While this PR could continue in it's direction, modifying keyframes to flip directions... the aliases are really nice, and really.. we might be able to get away with swapping the references ourself:

@keyframes slide-out-right {
  to { transform: translateX(100%) }
}

@keyframes slide-out-left {
  to { transform: translateX(-100%) }
}

html {
  --animation-slide-out-right: slide-out-right .5s var(--ease-3);
  --animation-slide-out-left: slide-out-left .5s var(--ease-3);
  --animation-slide-out-inline-end: var(--animation-slide-out-right);
  --animation-slide-out-inline-start: var(--animation-slide-out-left);

  &[dir=rtl] {
    --animation-slide-out-inline-end: var(--animation-slide-out-left);
    --animation-slide-out-inline-start: var(--animation-slide-out-right);
  }
}

do you think this is better? this also means no breaking changes, no changes to keyframes, just new logical animation keywords. we could do the same for the block direction. feels less complicated in both managing the source and reading the source output (as an author). thoughts?

&:dir(rtl) {
/* PostCSS compiles this to [dir="rtl"] :where(html) for older browsers, which doesn't work */
Copy link
Owner

Choose a reason for hiding this comment

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

adding 'dir-pseudo-class': false, to https://github.com/argyleink/open-props/blob/main/postcss.config.cjs#L17 will resolve this. postcss is attempting to polyfill it and we want just nesting.

}

&[dir="rtl"] {
/* Force the dir attribute selector to attach to :where(html) */
Copy link
Owner

Choose a reason for hiding this comment

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

this is a nice backup to using :dir() anyway, we'll do it ourselves in our own nesting way and not use the polyfill that's part of postcss-preset-env

Comment on lines +13 to +16
--animation-slide-out-end: slide-out-end .5s var(--ease-3);
--animation-slide-out-right: var(--animation-slide-out-end);
--animation-slide-out-start: slide-out-start .5s var(--ease-3);
--animation-slide-out-left: var(--animation-slide-out-start);
Copy link
Owner

Choose a reason for hiding this comment

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

nice job matching these all up 👍🏻

@ryanmitchx
Copy link
Author

forgot about block 😔 easy enough to fix though

I think I ultimately landed on flipping around the keyframes because it allows us to use/follow the --isLTR prop. I don't really know if there's a use-case for this, but I could see someone overriding the --isLTR prop (maybe on a per-page or per-component basis) and wanting that to flip directions for logical props. It also works around the issue where I'm not sure how to add the &[dir=rtl] selector into https://github.com/argyleink/open-props/blob/main/src/props.supports.js and get it to compile to CSS without modifying the build scripts further 🫣

@ryanmitchx
Copy link
Author

I guess the other thing I didn't think about is vertical languages. I'm not even sure how to go about detecting that in CSS, though

Copy link
Owner

@argyleink argyleink left a comment

Choose a reason for hiding this comment

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

i think you've done the best you can given the makeshift build process setup available! i should make it easier for modules to specify selectors and variable value exceptions. make that build file less messy and free up the modules.

a couple dx questions:

  1. does inline need to be included in the name so it's not ambiguous?
  2. should the docsite show both the physical and logical animations? like right now, it's always logical, even if i say --animation-slide-in-right?

#2 would mean moving away from the --isLTR strategy and towards that flipped alias one from #422 (review)

Comment on lines +2470 to +2471
--animation-slide-out-{up,down,start,end}
--animation-slide-in-{up,down,start,end}
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
--animation-slide-out-{up,down,start,end}
--animation-slide-in-{up,down,start,end}
--animation-slide-out-{up,down,left,right,start,end}
--animation-slide-in-{up,down,left,right,start,end}

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.

2 participants