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.

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: make MkImgWithBlurhash transitionable #10500

Merged
merged 14 commits into from
Apr 29, 2023
Merged

Conversation

acid-chicken
Copy link
Member

What

  • MkImgWithBlurhash の切り替わりをトランジションにできるように
  • MkGalleryPostPreview でホバー(→トランジション)中に画像の読み込みが始まるケースで見かけ上のトランジションを画像読み込み完了まで遅延させる

Why

トランジションが壊れているように見えるかもしれないため

Additional info (optional)

Checklist

  • Read the contribution guide
  • Test working in a local environment
  • (If needed) Update CHANGELOG.md
  • (If possible) Add tests

@github-actions github-actions bot added the packages/frontend Client side specific issue/PR label Apr 7, 2023
@github-actions github-actions bot requested a review from rinsuki April 7, 2023 04:49
@codecov
Copy link

codecov bot commented Apr 7, 2023

Codecov Report

Merging #10500 (e220c07) into develop (5124db5) will increase coverage by 2.31%.
The diff coverage is 95.55%.

@@             Coverage Diff             @@
##           develop   #10500      +/-   ##
===========================================
+ Coverage    76.08%   78.39%   +2.31%     
===========================================
  Files          898      165     -733     
  Lines        88742    20614   -68128     
  Branches      6268      374    -5894     
===========================================
- Hits         67517    16161   -51356     
+ Misses       21225     4453   -16772     
Impacted Files Coverage Δ
...ages/frontend/src/components/MkImgWithBlurhash.vue 91.97% <95.55%> (+3.08%) ⬆️

... and 733 files with indirect coverage changes

@acid-chicken acid-chicken requested review from syuilo and removed request for rinsuki April 9, 2023 04:18
@tamaina
Copy link
Contributor

tamaina commented Apr 12, 2023

Transitionの中でv-ifとv-showの同時使用はできないっぽい?

@tamaina
Copy link
Contributor

tamaina commented Apr 12, 2023

#10452 とちゃんぽんしたらblurhashから動かない

@tamaina
Copy link
Contributor

tamaina commented Apr 12, 2023

単にマージミスってるわ

@tamaina
Copy link
Contributor

tamaina commented Apr 12, 2023

#10452 といい感じにちゃんぽんしたいけど考えるのがだるい

@tamaina
Copy link
Contributor

tamaina commented Apr 12, 2023

(トランジションいらないなら #10452 でいいんじゃね説すらある?)

@acid-chicken
Copy link
Member Author

(トランジションいらないなら #10452 でいいんじゃね説すらある?)

トランジションいらないってどういうことかよくわからんけど v-show 含めずに width height いじったらマージできない?

@tamaina
Copy link
Contributor

tamaina commented Apr 12, 2023

#10452 ではcover/containを画像と同一にしたのでトランジションが壊れているように見えることはなさそう

あと#10452は高さ可変を実現するために position: absoluteを外してあるのでちょっとフラッシュが発生するなどする

@acid-chicken
Copy link
Member Author

あー

#10452 ではcover/containを画像と同一にしたのでトランジションが壊れているように見えることはなさそう

トランジションが壊れているっていうのは

  • MkGalleryPostPreview でホバー(→トランジション)中に画像の読み込みが始まるケースで見かけ上のトランジションを画像読み込み完了まで遅延させる

の際にホバー中に画像読み込みが走るのでタイミング如何でフラッシュすることを指している

@tamaina
Copy link
Contributor

tamaina commented Apr 12, 2023

ホバーしたらすぐに表示したい画像は先に読み込んでおくのが楽じゃない…?

@acid-chicken
Copy link
Member Author

#10478 より前の仕様に従っていて、で #10478 マージ後もどうせ必要になる

@tamaina
Copy link
Contributor

tamaina commented Apr 12, 2023

#10478 マージ後もどうせ必要に

そう?

@tamaina
Copy link
Contributor

tamaina commented Apr 12, 2023

外側でやってた切り替えをforceBlurhashで制御するってことかしら

@acid-chicken
Copy link
Member Author

データセーブ時は本当に必要になるまで画像を読み込みたくなくて、愚直にやると先ほど述べたようなトランジションエラーがある
一応外側で画像を読み込んでやれば解決するがなんか違くねという気持ち

@tamaina
Copy link
Contributor

tamaina commented Apr 12, 2023

トランジションエラーに遭遇したことがないのでよくわかってない(imgの読み込み遅延には遭遇するけどフラッシュするというのはちょっと違うような)

一応外側で画像を読み込んでやれば解決するがなんか違くねという気持ち

わかる

どれをマージしてどれをマージしないかは私は判断できない(しゅいろが選ぶこと)な気がするのでこれ以上マージについて考えるのが面倒

@acid-chicken
Copy link
Member Author

acid-chicken commented Apr 12, 2023

トランジションエラーに遭遇したことがないのでよくわかってない(imgの読み込み遅延には遭遇するけどフラッシュするというのはちょっと違うような)

https://6428f7d7b962f0b79f97d6e4-ttmyripqts.chromatic.com/?path=/story/components-mkgallerypostpreview--sensitive で回線速度落とすと再現できる

初回のホバーと二回目以降のホバーの挙動が違うのはエラーと解釈して良さそう

どれをマージしてどれをマージしないかは私は判断できない(しゅいろが選ぶこと)な気がするのでこれ以上マージについて考えるのが面倒

まぁそう

@tamaina
Copy link
Contributor

tamaina commented Apr 12, 2023

コンポーネントの内側のトランジションの内容を外側から操作するとしたらどうするんだろうか

@acid-chicken
Copy link
Member Author

トランジションにかける CSS をカスタマイズ可能にするってこと?

@tamaina tamaina changed the title feat: make MkImgWithBlurhash animatable feat: make MkImgWithBlurhash transitionable Apr 14, 2023
@acid-chicken
Copy link
Member Author

数日待って返答がなかったのでそうした

@tamaina
Copy link
Contributor

tamaina commented Apr 16, 2023

普通に気づいてなかった

トランジションにかける CSS をカスタマイズ可能にするってこと?

そのほうが色々やりやすそう

@tamaina
Copy link
Contributor

tamaina commented Apr 28, 2023

forceBlurhashを使ってMkMediaImageをいい感じに書き換えられないかしら

@acid-chicken
Copy link
Member Author

ここでまとめてやらんでもよくね

@tamaina
Copy link
Contributor

tamaina commented Apr 28, 2023

思った通りposition: absoluteのせいで伸縮機能が完全に機能してない

@acid-chicken
Copy link
Member Author

どこのこと?

@tamaina
Copy link
Contributor

tamaina commented Apr 28, 2023

MkMediaImageまわり

@tamaina
Copy link
Contributor

tamaina commented Apr 28, 2023

修正しております

@tamaina
Copy link
Contributor

tamaina commented Apr 28, 2023

いろいろ修正した

@tamaina tamaina merged commit 9d5911d into develop Apr 29, 2023
@tamaina tamaina deleted the blurhash-transition branch April 29, 2023 13:57
@tamaina
Copy link
Contributor

tamaina commented Apr 29, 2023

とりあえずマージしたんだけど、canvasのコンテキストパージを明示しないとメモリリークする可能性あるんじゃねと思った

@tamaina
Copy link
Contributor

tamaina commented Apr 29, 2023

87657d0 をプッシュし忘れたのでdevelopに突っ込んだ…

sasagar pushed a commit to sasagar/misskey that referenced this pull request May 9, 2023
* feat: make `MkImgWithBlurhash` animatable

* refactor: split out transition styles

* fix: bug

* test: waitFor image loads

* style: remove unused await

* fix

* fix type error

---------

Co-authored-by: tamaina <[email protected]>
na2na-p pushed a commit to na2na-p/misskey that referenced this pull request May 10, 2023
* feat: make `MkImgWithBlurhash` animatable

* refactor: split out transition styles

* fix: bug

* test: waitFor image loads

* style: remove unused await

* fix

* fix type error

---------

Co-authored-by: tamaina <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
packages/frontend Client side specific issue/PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants