-
Notifications
You must be signed in to change notification settings - Fork 40
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
feat(new-ui): add image prefetch in new ui #628
base: master
Are you sure you want to change the base?
feat(new-ui): add image prefetch in new ui #628
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Из важных замечаний у меня 2 — вместо коммента в старом коде просто заюзать новую функцию + переименовать preload в cleanup, чтобы было правдиво
В остальном считаю получилось классно, потестил с разным инет, все прелоадится, крутяк!
@@ -160,6 +160,10 @@ export function parseKeyToGroupTestsBy(key) { | |||
return [groupSection, groupKey]; | |||
} | |||
|
|||
/** @deprecated - this is just not working. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Так давай вместо коммента просто удалим эту функцию и будем юзать везде новую, если она работает корректно и лучше?
}, [currentNamedImageIndex]); | ||
|
||
useEffect(() => () => { | ||
Object.values(preloaded.current).forEach(preload => preload?.()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ну это же не preload()
получается, а cleanup какой-нибудь? нейминг немного сбил с толку
orderedImages[i] | ||
); | ||
} | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Может еще докинуть тест с кликом на кнопку, что грузится еще 1 картинка? Но не критично
}, [currentNamedImageIndex]); | ||
|
||
useEffect(() => () => { | ||
Object.values(preloaded.current).forEach(preload => preload?.()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Еще на всякий случай хочу спросить, мы получается для прелоада добавляем прям элементы в head и только на unmount их удаляем. Если туда напихать скажем так 5000 скриншотов, ничего плохого не будет, т.е. браузер будет понимать, что это лишь rel=preload и не будет реально их все держать в памяти, верно?
Еще там линтер ругается на код тестов. Предлагаю или починить warning'и или заигнорить их. |
Added image prefetching in new ui
I hereby agree to the terms of the CLA available at: https://yandex.ru/legal/cla/?lang=ru.