Conversation
🦋 Changeset detectedLatest commit: 4238db0 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Bundle size report
|
Pull Request Test Coverage Report for Build 22579831996Details
💛 - Coveralls |
hextion
force-pushed
the
feat/lottie
branch
4 times, most recently
from
739f76d to
9dcb40e
Compare
Demo build (default) |
Demo build (alfasans) |
hextion
force-pushed
the
feat/lottie
branch
3 times, most recently
from
1ad4b85 to
87fc4f0
Compare
hextion
force-pushed
the
feat/lottie
branch
2 times, most recently
from
53bfe40 to
2a0ca51
Compare
hextion
requested review from
SiebenSieben,
dHIM24 and
fulcanellee
as code owners
Snapshot release (default)Successfully released the following packages:
|
Snapshot release (alfasans)Successfully released the following packages:
|
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
SiebenSieben
left a comment
SiebenSieben
left a comment
There was a problem hiding this comment.
Отсутствуют unit-тесты. В библиотеке это обязательное требование для каждого компонента — нужен Component.test.tsx. Без тестов сложно гарантировать корректность поведения при будущих изменениях.
| export const lottie: Story = { | ||
| name: 'Lottie', | ||
| render: () => { | ||
| return <Lottie animation={{ path: 'http://localhost:9009/twitter-heart.json' }} />; |
There was a problem hiding this comment.
Хардкод localhost — в задеплоенном Storybook это сломается. Нужно использовать относительный путь, аналогично тому как сделано в description.mdx (./twitter-heart.json).
| direction={direction} | ||
| speed={1} | ||
| iterations={0} | ||
| direction={direction} |
There was a problem hiding this comment.
Дублирующийся проп direction={direction}. JSX не выдаст ошибку компиляции в MDX, но второй проп перезаписывает первый — нужно убрать одно из вхождений.
| [events, reset], | ||
| ); | ||
| useImperativeHandle(playCountRef, () => animation?.playCount ?? 0, [animation]); | ||
|
|
There was a problem hiding this comment.
useImperativeHandle предназначен для кастомизации хендла, прокидываемого наружу через forwardRef. Использование его на внутреннем playCountRef нестандартно и вводит в заблуждение. Достаточно обычного эффекта:
useEffect(() => {
playCountRef.current = animation?.playCount ?? 0;
}, [animation]);| 'max-params': 'off', | ||
| 'react-hooks/exhaustive-deps': | ||
| lottiePkg && isSamePath(process.cwd(), lottiePkg.dir) | ||
| ? ['warn', { additionalHooks: ['useLayoutEffect_SAFE_FOR_SSR'].join('|') }] |
There was a problem hiding this comment.
Два вопроса по этому изменению:
-
useLayoutEffect_SAFE_FOR_SSRиз@alfalab/hooksиспользуется во всём репозитории, не только вlottie. Если хук нужно добавить вadditionalHooks— логичнее сделать это глобально для всех пакетов, а не только приisSamePath(..., lottiePkg.dir). -
["useLayoutEffect_SAFE_FOR_SSR"].join("|")возвращает просто"useLayoutEffect_SAFE_FOR_SSR"—.join("|")на массиве из одного элемента избыточен.
| animationItem.destroy(); | ||
| // eslint-disable-next-line no-underscore-dangle | ||
| animationItem._cbs = []; | ||
| setAnimation(null); |
There was a problem hiding this comment.
Прямая запись в приватное поле _cbs обходит публичный API lottie-web. При обновлении библиотеки это поле может исчезнуть или изменить структуру, и баг будет трудно отловить. Если это необходимо для предотвращения утечек памяти — стоит добавить подробный комментарий, почему это нельзя сделать через animationItem.destroy() или другой публичный способ.
| "tslib": "^2.4.0" | ||
| }, | ||
| "peerDependencies": { | ||
| "react": "^16.9.0 || ^17.0.1 || ^18.0.0", |
There was a problem hiding this comment.
Все остальные пакеты в репозитории уже добавили поддержку React 19. Нужно привести в соответствие:
"react": "^16.9.0 || ^17.0.1 || ^18.0.0 || ^19.0.0",
"react-dom": "^16.9.0 || ^17.0.1 || ^18.0.0 || ^19.0.0"| </div> | ||
| ); | ||
| }, | ||
| ); |
There was a problem hiding this comment.
Не хватает displayName — это стандарт библиотеки (см. Button, Checkbox и др.):
Lottie.displayName = "Lottie";| * Дополнительный класс | ||
| */ | ||
| className?: string; | ||
| } |
There was a problem hiding this comment.
В библиотеке принято поддерживать dataTestId во всех компонентах для использования в автотестах. Нужно добавить проп и прокинуть через data-test-id на корневой элемент.
5 participants
No description provided.