doxas / twigl

twigl.app is an online editor for One tweet shader, with gif generator and sound shader, and broadcast live coding.
https://twigl.app/
MIT License
803 stars 50 forks source link

Feature: Snapshot #41

Closed 0b5vr closed 1 year ago

0b5vr commented 1 year ago

Depends on #40

説明

"Generate LINK" の改善です。

"snapshot" と称して、シェーダのコードをDBに保存するようにしました。 これにより、より長いシェーダコードのURLを発行できるようになります。 DBのスキーマについては、 database-structure.md を参照ください。

ローカル環境で動かす際は、以下のURLが読めるかを試してみてください: http://localhost:9090/?ol=true&ss=-NJuMx71uJfXhztXkOLl

レビュー観点

doxas commented 1 year ago

@0b5vr

いまローカルで動かそうとしているところで、以下のとおりのエラーが出てます。 image

これどうやら async/await 関係っぽいのですが、そちらの環境ではグローバルに入ってるなにかが作用してうまく動いちゃってるんですかね……(そういう可能性ありますか?)

私の方で babel の plugin みたいなの追加で入れるべきなのか、各種パッケージのバージョンを更新するような全体的な刷新作業をするべきなのか迷いますが、ひとまずいったんエラーが出ないようになにかしらの対策を行い、ローカルで動く状態にして変更していただいた内容を踏まえた挙動を見てみようと思ってます。 そもそも async/await を使うことを制限するというのは現代的ではないですし考えてません。

0b5vr commented 1 year ago

いまローカルで動かそうとしているところで、以下のとおりのエラーが出てます。

これどうやら async/await 関係っぽいのですが、そちらの環境ではグローバルに入ってるなにかが作用してうまく動いちゃってるんですかね……(そういう可能性ありますか?)

あーーすいません、いままでyarnでインストールしてて、npm使ったら再現しました。ちょっとpackage-lock.jsonのご機嫌取りをしないといけなさそうですね。

0b5vr commented 1 year ago

全体のリファクタリングしてない件、たぶんリファクタリングするなら TypeScript にしちゃったほうがいいんでしょうね……ちょっといまそれをやるの難しいので、現時点での進行としてはその方針でまったく問題ございません

TypeScriptに切り替えずとも、ビュー周りを中心に機能を少しずつ script.js から引き剥がしていくときれいになっていきそうな印象です。

0b5vr commented 1 year ago

試しに npm upgrade でごっそりアップグレードしてみたら、それでうまく行った気がします(yarn叩いてうまく行っていたので、それと同じ状態だとおもいます)。 一旦それでpushしてみてよいでしょうか?

doxas commented 1 year ago

こちらではまだ全然検証に着手できてなかったので、助かります!

0b5vr commented 1 year ago

pushしてみました。これでnpm installし直して試してみてください。

doxas commented 1 year ago

@0b5vr

お返事遅くなってしまってすいません。 実際に動作させながら挙動をいろいろ確認してみましたが、一点相談したいです。

  1. Generate LINK する(シェーダ A)
  2. シェーダ A を別タブで開く
  3. favorite を連打して数を変える
  4. コードを改変するなどして再度 Generate LINK する(シェーダ B)
  5. ブラウザのアドレスバーはシェーダ B を指す URL になっているが fav などの数値はそのまま残った状態になる

Generate LINK の操作は、基本的に「常に新規レコードを追加」になると思うので、ビュー数と fav 数は一緒にリセットしちゃうのがよいのではないか、と思うのですがどうでしょう? その方針で良ければ私の方でその変更を加えることもできると思います!

0b5vr commented 1 year ago

ビューがリセットされてないという話ですね。それは忘れておりました!やります!

0b5vr commented 1 year ago

@doxas 遅れましたが上記対応しました! 他のリンクから飛んできた場合でないときは、そもそも視聴者数もスター数も表示されていない状態なので、Generate LINKを押したらそれらを消すようにしました。

doxas commented 1 year ago

@0b5vr

修正ありがとうございます。 挙動についても確認でき、問題ないと感じました。

date をどこかに表示するなら……というデザイン・スタイリングの話がペンディングになってたかと思いますが、今回の PR(または直近の変更・更新したい範囲)に保存日時の表示って含まれる感じですか? それとも、無きゃ無いで構わないという温度感でしょうか?

もし日時を何かしらの方法で表示したいということなら、code pane が出ているとき限定で chars の脇とかに少し隙間作って出すとかがいいのかな~と思いました。

image

published yyyy-mm-dd みたいにすればそんなに邪魔にもならないし良さそうな気はするけど、コードペイン無しの状態とかでも見れるようにということになると、どっか右下とかに浮かすしかないのかもしれず、それだったらまあ表示しなくてもいいのかなという気がします。

上記の日時表示どうするかだけ検討して、結論出たらデプロイはすぐにできると思います。

0b5vr commented 1 year ago

@doxas あーー、そこならアリかもな。帰ったら試してみます

0b5vr commented 1 year ago

@doxas 遅くなりましたが、こんな感じで表示してみました。

Date.toLocaleString() 使ってるので、環境のロケールに応じた日付文字が表示されるはずです。 ぼくの環境はイギリス英語になっています(ちなみにこの日付表記は嫌いです)。

image

doxas commented 1 year ago

@0b5vr

日時表示について実装していただいてありがとうございます! 外見ばっちりですね! ※わたしの環境は日本語環境だったので日付は yyyy/m/d で出てました

一点だけ、なんか重箱の隅をつつくようなことをしつこく言うようで申し訳ないのですが…… 現状の挙動だと、リンクを生成したときに broadcast 中だけ日時表示が消え、そうでない場合は日時表示が残るようになっています。

個人的には、リンクを生成したときに視聴者数や Like が非表示になるのと同時に日時についても同様に非表示化でよい気がしました。 もしくはその瞬間の時刻に更新する、とかのほうが UX 的には便利なんだろうか。 現状あえて broadcast 中だけ非表示化されるようになっている意図がもしあれば教えてほしいです!

doxas commented 1 year ago

@0b5vr

もし特に意図とかなくてそうなってしまってるだけとかなら、私の方でも直せると思うので言ってください!

0b5vr commented 1 year ago

@doxas 本当だ。ここはいずれにせよ投稿日時消えてもいいですね。 直しました!

0b5vr commented 1 year ago

memo: diffがデカいのは package-lock.json を更新したから

doxas commented 1 year ago

@0b5vr

ありがとうございます。 これで一通り調整できたと思いますのでひとまず merge してデプロイまでやってみます。