framer / motion

Open source, production-ready animation and gesture library for React
https://framer.com/motion
MIT License
22.41k stars 740 forks source link

update list of camel case svg attributes #1153

Closed KirdesMF closed 2 years ago

KirdesMF commented 3 years ago

1152

it should be good now

codesandbox-ci[bot] commented 3 years ago

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 1c4f27c40bbf4d0a8a3504b4b29ef5f0a2d841ae:

Sandbox Source
Framer Motion: Simple animation Configuration
App Store UI using React and Framer Motion Configuration
Framer Motion: Reorder animation Configuration
Framer Motion: growing item positionTransition issue Configuration
Framer Motion: Image lightbox Configuration
KirdesMF commented 3 years ago

https://codesandbox.io/s/framer-motion-simple-animation-forked-j53vx?file=/src/Example.tsx

mattgperry commented 2 years ago

Thanks for this. I'm wondering though, given how obscure all these attr names are, if there's a way we could get rid of the array altogether.

rijk commented 2 years ago

@mattgperry could we merge this in the meantime? Because it's currently not possible to animate these properties.

rijk commented 2 years ago

Hm, I found that monkey patching this array didn't fix the issue for me.. Debugging, it seems renderSVG is not called at all? 🤔

mattgperry commented 2 years ago

@rijk Is this still true with the latest version of Motion? Which element and attribute were you animating?

Closing this out though as I'd rather these quite obscure props weren't animatable than add the extra bundlesize.

rijk commented 2 years ago

Hi @mattgperry ! Yup, I am using 6.3.3 and it was not working. I was trying to animate startOffset for the scroll-triggered marquee text on this site. But when I tried to do this:

<motion.textPath href="#path" startOffset={offset}>
  {children}
</motion.textPath>

it didn't work because it incorrectly converts the attribute to start-offset. So I had to resort to this:

useEffect(() => {
  return offset.onChange((v) => {
    path.current?.setAttribute('startOffset', v)
  })
}, [])

which works but I think doesn't perform as well.

rijk commented 2 years ago

Very annoying that the SVG attributes are so inconsistent.. I was thinking you could maybe assume camelCase when in an SVG context, but glyph-orientation-vertical vs glyphRef, marker-start vs markerWidth.. there is no logic to the naming it seems they just flipped a coin 🤔

Edit: ahh, the dash-case ones are apparently what they call "Presentation attributes". That is why they are not camelCased:

SVG presentation attributes are CSS properties that can be used as attributes on SVG elements.

rijk commented 2 years ago

So knowing this, I think a way to do it without bloating the bundle with a hardcoded list is to default to camelCase for SVG, and only use dash-case (not sure what the official name for that is) for these CSS presentational attributes;

const svg = document.createElement('svg')
const key = attribute in svg.style ? property : camelCase(attribute)

Or more to the point of the code:

key = camelToDash(key) in domElement.style ? camelToDash(key) : key
mattgperry commented 2 years ago

That's very clever. I wonder how it can be done with SSR in mind. I think this is a good starting place though.

rijk commented 2 years ago

Aren’t these functions (get/set current animation value) by their nature only run on the client?

mattgperry commented 2 years ago

Yeah but off the top of my head we also output JSX to SSR the initial state

bcavenagh-foxtrot commented 1 year ago

Just checking in here—was the startOffset one ever fixed? I'm having trouble animating it and am seeing start-offset still showing up in the inspector. Is the useEffect that @rijk wrote the only workaround for this right now?