Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[project-library-manage] ライブラリ管理画面で表示される画像及び音声をインターネット上のものを読み込めるようにし、遅延読み込みにする #1560

Merged
merged 5 commits into from
Sep 19, 2023
Merged

[project-library-manage] ライブラリ管理画面で表示される画像及び音声をインターネット上のものを読み込めるようにし、遅延読み込みにする #1560

merged 5 commits into from
Sep 19, 2023

Conversation

y-chan
Copy link
Member

@y-chan y-chan commented Sep 17, 2023 •
edited
Loading

内容

題のとおりです。

関連 Issue

スクリーンショット・動画など

その他

#1497 の挙動を破壊しているので、先にこちらをマージしたいです。

@y-chan y-chan requested a review from a team as a code owner September 17, 2023 05:24
@y-chan y-chan requested review from Hiroshiba and removed request for a team September 17, 2023 05:24
@y-chan y-chan mentioned this pull request Sep 17, 2023
Merged
@y-chan y-chan changed the title [project-library-manage]ライブラリ管理画面で表示される画像及び音声をインターネット上のものを読み込めるようにし、遅延読み込みにする [project-library-manage] ライブラリ管理画面で表示される画像及び音声をインターネット上のものを読み込めるようにし、遅延読み込みにする Sep 17, 2023
Hiroshiba
Hiroshiba reviewed Sep 17, 2023
View reviewed changes
Copy link
Member

@Hiroshiba Hiroshiba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ネットの画像も読み込めるようにするの、良いと思います！
普通にurlをsrcに書けばvue3-lazyloadがなくても遅延で動きそうですが、このライブラリを使う意図は別にあったりしますか 👀

@y-chan
Copy link
Member Author

y-chan commented Sep 17, 2023

Lazy Loadできると遅延読み込みになる分、画面外に表示されるような不要な画像の読み込みを一時的に(スクロールとかされると結局読み込まれるので)削減できたり、あるいはリクエストを分散できたりするので、ユーザーにとってレスポンスがより早く感じられるかなと言った感じです。
メンテナンスの観点として依存するライブラリは増えるので、不要なら戻そうかなーと...!

@Hiroshiba
Copy link
Member

@y-chan あ、なるほどです！
imgは何もしなくてもlazyだと思っていました。。

調べてみた感じ、もしかしたらHTML側で普通にloading=lazyがサポートされているかもです。
https://developer.mozilla.org/ja/docs/Web/HTML/Element/img#loading

@y-chan
Copy link
Member Author

y-chan commented Sep 17, 2023

試してみた感じ、HTMLのloading="lazy"で十分そうでした！
vue3-lazyloadに依存しなくても良さそうだったので消しました...!
また、portraitの方は遅延読み込みにする意味がなさそうだったので、消しました...!

Hiroshiba
Hiroshiba approved these changes Sep 19, 2023
View reviewed changes
Copy link
Member

@Hiroshiba Hiroshiba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM！

１箇所コメントしてるのでそこだけ！

src/components/CharacterTryListenCard.vue Outdated Show resolved Hide resolved
src/components/LibraryManageDialog.vue
Comment on lines +278 to +280
portraitPath: isValidHttpUrl(portrait)
? portrait
: base64ImageToUri(portrait),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

今に始まった話じゃないですが、pathにURLやURIが入ってるの良くないですね･･･ 😇

Hiroshiba
Hiroshiba reviewed Sep 19, 2023
View reviewed changes
Copy link
Member

@Hiroshiba Hiroshiba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

問題ないと思うのでマージします！

エンジン側の話ですが、portraitとかにパス以外の値（URL）が入るように仕様変更するのであればドキュメントの変更もしていく必要があるなと思いました！

@Hiroshiba Hiroshiba merged commit 8daf10e into VOICEVOX:project-library-manage Sep 19, 2023
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Hiroshiba Hiroshiba Hiroshiba approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@y-chan @Hiroshiba