DaveKeehl / svelte-reveal

Svelte action that leverages the Intersection Observer API to trigger reveal on scroll transitions.
https://stackblitz.com/edit/svelte-reveal?file=src%2FApp.svelte
MIT License
131 stars 3 forks source link

Switch to only transition style #129

Closed nathancahill closed 2 years ago

nathancahill commented 2 years ago

Per #128

gitpod-io[bot] commented 2 years ago

nathancahill commented 2 years ago

I'd also remove the addVendor function since transition transform etc are supported in all browsers now: https://caniuse.com/?search=transition

If for some reason they need to be added, there's CSS tooling like autoprefixer that can add them if the user desires.

DaveKeehl commented 2 years ago

It took me a second to remember why I had added those CSS rules. The problem is that if I do not set display: block on the element, then the scale and spin animations are not relative to the element and do not use it as the transform-origin position, creating very unpleasant animations. You would for example see the element spinning 360 all over the page or sliding in from the right edge of the screen.

I do understand that it causes other CSS layouts to break, but at the moment I don't think that removing those styles is best solution.

Thank you also for the addVendor suggestion, I will definitely consider that!

DaveKeehl commented 2 years ago

Do you have a repository with an example that breaks that I can take a look at?

nathancahill commented 2 years ago

Sure, here's a basic example: https://svelte.dev/repl/cc5a96d591c94de4ac6edcba92f5f465?version=3.47.0

The div has a flex layout. reveal overrides it with display: block so the two h1 tags are displayed incorrectly. Again, like any other layout, if the user needs display: block they can add it easily. Shouldn't be the responsibility of the library that's in charge of the reveal functionality.

DaveKeehl commented 2 years ago

That's a valid point. I noticed that actually just by using width: fit-content without setting any display property on the element, the animations work fine anyway. I will make sure to write in the documentation that for some animations it is required to add a bit of CSS.

You can expect a new minor release very soon. Probably either today or tomorrow.

codecov[bot] commented 2 years ago

Codecov Report

Merging #129 (91e01bc) into develop (22db124) will not change coverage. The diff coverage is n/a.

@@           Coverage Diff            @@
##           develop     #129   +/-   ##
========================================
  Coverage    82.99%   82.99%           
========================================
  Files           14       14           
  Lines          347      347           
  Branches        60       60           
========================================
  Hits           288      288           
  Misses          54       54           
  Partials         5        5           
Impacted Files Coverage Δ
src/internal/styling/classesGeneration.ts 100.00% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 22db124...91e01bc. Read the comment docs.