-
Notifications
You must be signed in to change notification settings - Fork 44
feat(messages): コンテキストメニューやスタンプピッカーが表示されている MessageElement も背景色を強調する
#4841
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
Conversation
|
Preview (prod) → https://4841-prod.traq-preview.trapti.tech/ |
MessageElement も背景色を強調するMessageElement も背景色を強調する
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #4841 +/- ##
==========================================
- Coverage 13.54% 13.53% -0.02%
==========================================
Files 704 704
Lines 31692 31717 +25
Branches 677 677
==========================================
Hits 4294 4294
- Misses 27386 27411 +25
Partials 12 12 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
もう一回考えます :bow-nya2: |
|
|
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.
動作としてはちゃんと動いてて良さそうです
設計の根本に関わるコメントをしたので読んでもらえると嬉しいです
| :class="$style.body" | ||
| :data-is-mobile="$boolAttr(isMobile)" | ||
| :data-is-editing="$boolAttr(isEditing)" | ||
| :data-is-active="$boolAttr(!!contextMenuPosition)" |
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.
[imo] contextMenuPosition !== undefined の方が分かりやすいかも
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.
isDefined を使いました
| const emit = defineEmits([ | ||
| 'toggleStampPicker', | ||
| 'openContextMenu', | ||
| 'closeContextMenu' | ||
| ]) |
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.
[should] 積極的な理由がなければ defineEmits<{ (e: 'event'): void }>() みたいに書いて欲しい
| const isActive = computed( | ||
| () => isStampPickerOpen.value || !!contextMenuPosition.value | ||
| ) |
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.
この値を取るためだけに必要以上に MessageTools のロジックが露出しているように感じるな すごくきれいというわけでもないけど、MessageTools でこの computed を持って effect -> emit で同期するというのが考えられると思うけど、どうでしょう?
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.
他の実装に合わせて v-model を使ってみました 👀
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.
3e2b7ce to
c3c58f2
Compare
- `MessageElement` の挙動と一致しない部分を修正
c3c58f2 to
40969ec
Compare

概要
MessageElementも背景色を強調するなぜこの PR を入れたいのか
close: #3981
UI 変更部分のスクリーンショット
PR を出す前の確認事項