feat: Resolve benchmark github action #1886
Conversation
cieplypolar
requested review from
aleksanderkatan,
iwoplaza,
lursz and
reczkok
|
pkg.pr.new packages benchmark commit |
There was a problem hiding this comment.
I see that the page does not respond to the light theme, but /benchmark acts the same so we may ignore it (at least until the landing page rework)
apps/typegpu-docs/astro.config.mjs
Outdated
| image: { | ||
| domains: ['raw.githubusercontent.com'], | ||
| }, |
There was a problem hiding this comment.
Why do we need that?
There was a problem hiding this comment.
Actually, we don't, my bad
| // this is for infinite effect | ||
| const extendedPlots = useMemo( | ||
| () => [plots[plots.length - 1], ...plots, plots[0]], | ||
| [], | ||
| ); |
There was a problem hiding this comment.
I think we don't really need this - creating logic for wrapping in callbacks is trivial and we don't care about the dependency (plots never changes).
There was a problem hiding this comment.
I don't see a simple solution inside the callbacks, if I just use modulo length, then the carousel will go through all the plots
| ); | ||
|
|
||
| const [currentIndex, setCurrentIndex] = useState(1); | ||
| const [isTransitioning, setIsTransitioning] = useState(false); |
There was a problem hiding this comment.
| const [isTransitioning, setIsTransitioning] = useState(false); | |
| const isTransitioningRef = useRef(false); |
I would go for this - isTransitioning does not need to cause re-renders and it will simplify transition handling
| const nextSlide = useCallback((isTransitioning: boolean) => { | ||
| if (isTransitioning) return; | ||
| setIsTransitioning(true); | ||
| setCurrentIndex((prev) => prev + 1); // to avoid deps | ||
| }, []); |
There was a problem hiding this comment.
+ const slideCount = plots.length;| const nextSlide = useCallback((isTransitioning: boolean) => { | |
| if (isTransitioning) return; | |
| setIsTransitioning(true); | |
| setCurrentIndex((prev) => prev + 1); // to avoid deps | |
| }, []); | |
| const nextSlide = useCallback(() => { | |
| if (isTransitioningRef.current) return; | |
| isTransitioningRef.current = true; | |
| setCurrentIndex((prev) => prev === slideCount - 1 ? 0 : prev + 1); | |
| }, [slideCount]); |
| const handleTransitionEnd = useCallback((index: number) => { | ||
| setIsTransitioning(false); | ||
|
|
||
| if (index === 0) { | ||
| setCurrentIndex(plots.length); | ||
| } else if (index === extendedPlots.length - 1) { | ||
| setCurrentIndex(1); | ||
| } | ||
| }, [extendedPlots]); |
There was a problem hiding this comment.
This will also be much simpler if we just do transitioning as a ref and the logic for wrapping in callbacks
| const handleTransitionEnd = useCallback((index: number) => { | |
| setIsTransitioning(false); | |
| if (index === 0) { | |
| setCurrentIndex(plots.length); | |
| } else if (index === extendedPlots.length - 1) { | |
| setCurrentIndex(1); | |
| } | |
| }, [extendedPlots]); | |
| const handleTransitionEnd = useCallback(() => { | |
| isTransitioningRef.current = false; | |
| }, []); |
There was a problem hiding this comment.
This is a little over-engineered but since it's for dev I don't really mind it (all the comments are just general feedback for learning purposes).
- We treat plots like they are not constant but they are so we should use it (for example there is no need to do useMemo for the plots - if we wanted to do extended plots we could just do that out of the component)
- There is no expensive calculation going on here so the optimizations are a little overkill but good practice
- Rest of the feedback is in the other comments
There was a problem hiding this comment.
I appreciate the feedback. I deleted useMemo, but I can't change isTransitioning to useRef. isTransitioning state plays a crucial role in smooth carousel wrap when we transition from the last plot -> to the first plot (and first -> last).
- Because plots are extended with 'guard' plots, the animation appears to move continuously to the right (left).
- Then, I handle the transform property. I set
isTransitioningto false, and change the index to real plot (not the guarding one) - The CSS (this one containing animation logic) rerenders and we don't see the carousel going through all the plots.
No description provided.