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

share-target でデフォルトテキストが表示されないバグの修正 #4163

Merged
merged 4 commits into from
Nov 27, 2023

Conversation

Nagarei
Copy link
Contributor

@Nagarei Nagarei commented Nov 23, 2023

share-target API が今まではデフォルトテキストが空文字になっていたので修正。
元々の状態でもテキストエリアに表示されないだけで編集せずに投稿すれば元々の文字列が投稿できていたが、一度でも編集するとテキストエリアの文字が投稿されていた。

Vue何も分からずに適当に編集しているので作法とか間違ってるかも

share-target APIの使用例: https://q.trap.jp/share-target?title=foo&text=aaaaaaaa&url=www.google.co.jp

Copy link

Copy link

codecov bot commented Nov 23, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (016a60a) 86.35% compared to head (ea4d834) 86.35%.
Report is 2 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #4163   +/-   ##
=======================================
  Coverage   86.35%   86.35%           
=======================================
  Files          66       66           
  Lines        4719     4719           
  Branches      564      564           
=======================================
  Hits         4075     4075           
  Misses        638      638           
  Partials        6        6           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@mehm8128 mehm8128 left a comment

Choose a reason for hiding this comment

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

PRありがとうございます、こんなページあったの始めて知りました

ShareTargetForm.vueの68~76行目の

// FIXME: 親子関係なのにprovide-injectを乱用してるの微妙
const { state } = useMessageInputState('share-target')
watch(
  () => props.defaultText,
  newVal => {
    state.text = newVal
  },
  { immediate: true }
)

でおそらくtextareaの値を初期化する処理は入っていそうなので、defaultTextをpropsとして渡す必要はなさそうです(useMessageInputStateshare-targetというkeyでstoreを通してコンポーネント間でstateが共有されていそうです)

なんでdefaultTextがtextareaに反映されていないのか色々見てみたのですが、なんでか分からないけどtextareav-if="state.text !== ''"みたいなのをつければ直りました(多分上に挙げたコードでnewValにしたのがtextareaの方には上手くリアクティブに反映されてなかったんだと思います)
ただ、v-ifを使う方針だとtextのquery paramが空文字orない場合だとtextareaが一生表示されなくなってしまうので、以下のどれかになりそうです

  • textのquery paramが空文字orない場合」がありえないと仮定していいならv-ifを使う方針にする
  • useMessageInputStateで変更がリアクティブに反映されるように修正する
    • useMessageInputStateの第二引数にdefaultTextを渡すとかでもいいかも?他のところでこのcomposablesがどう使われているのか把握してないのでもしかしたらよくないかもです
  • useMessageInputStateを使わずに、props・emitだけで処理するようにする
  • 今の方針のままにする
    • その場合、上で挙げたwatchのコードは消してもよさそうかもです

@Nagarei
Copy link
Contributor Author

Nagarei commented Nov 23, 2023

ShareTargetForm.vue の 77行目で使っている usePostMessage が、useMessageInputStateによって登録されたメッセージを投稿する関数に見えるので、useMessageInputState を使わないのは不自然になってしまう気がします。
同じ理由で state.text の初期化はしないといけないのでwatchも消せなくて、もし消した場合は無編集で送ると空文字が投稿されてしまう気がします。(確認しないで言っています)

useMessageInputState で変更がリアクティブに反映されるように修正するのが一番筋が良い気がしますが、実現方法が良く分からないです。

なお、usePostMessageは通常のチャンネルUI(src/components/Main/MainView/MessageInput/MessageInput.vue)上でのメッセージの投稿にも使っている関数のようです。ただし、第二引数の inputStateKey はこの share-target API でしか使っていないので、useMessageInputState を使わないオプションもとれるように修正してしまうという方法もあるかもしれないです。将来的に画像に対応したりとかまで考えると難しいかもしれない。

実装方針はメンテナでも vue 詳しいわけでも無いので僕からはなんとも...

@SSlime-s
Copy link
Member

色々試してみてたんですが、

- v-model="state.text"
+ :value="state.text"
+ @input="e => { state.text = (e.target as HTMLTextAreaElement).value }"

だけで治りそうでした (v-model と変わらないはずなのに :doushite:)

watch に console.debug とか仕込んで見た感じだと state 自体はリアクティブに変更されていて、その中身もちゃんといい感じに更新されてそうでした
useMessageInputState の処理の関係上 中で watch がいっぱい呼び出されるようになってて、そのサイクルと textareav-model に与えてる値が更新されたときの textarea の中での更新のサイクル がおかしくなってるのかなぁ と思ったんですが、試す術とかがわからず挫折したという記録だけ書いておきます

@mehm8128
Copy link
Contributor

↑で直りそうならそれでよさそうです

Copy link
Contributor

@mehm8128 mehm8128 left a comment

Choose a reason for hiding this comment

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

なんでこれで直るのかは結局分かってないけど、メインの機能でもないしいいと思います
ありがとうございます

@mehm8128 mehm8128 merged commit 07d2c54 into traPtitech:master Nov 27, 2023
10 of 11 checks passed
@Nagarei Nagarei deleted the fix/share-target-default-text branch January 26, 2024 08:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants