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

ノートを畳む条件を仮想行数、旧式、実際の表示の大きさから選べるようにする #13961

Open
wants to merge 12 commits into
base: develop
Choose a base branch
from

Conversation

FineArchs
Copy link
Contributor

@FineArchs FineArchs commented Jun 9, 2024

What

#13287@kakkokari-gtyih さんの #13761 をマージし、設定項目などを生やしました。
これにより、大きいノートが畳まれる条件を

  • 「高精度の算出方法」:(MFMの引数なども考慮する)仮想行数から算出、デフォルト
  • 「旧式の算出方法」:従来通り
  • 「実際の表示サイズに基づく」:clientHeightを参照

の三つから選べるようになります。

ついでに畳む大きさを大・中・小から選べるようにしています。

Why

#13266 で挙がっていた懸念点を

  • パフォーマンス上のリスク → 旧来の方法も選べるようにすることで最悪なんとかなるように
  • 畳む条件を考慮したMFMが作れなくなる → ある程度統一性のある算出方法をデフォルトにすることである程度意味のある考慮ができるように

という形である程度解決できるようにします。

Additional info (optional)

設定項目の名づけにあまり自信がありません

Checklist

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

@github-actions github-actions bot added the packages/frontend Client side specific issue/PR label Jun 9, 2024
Copy link

codecov bot commented Jun 9, 2024

Codecov Report

Attention: Patch coverage is 37.26708% with 202 lines in your changes missing coverage. Please review.

Project coverage is 77.23%. Comparing base (9849aab) to head (faf916d).

Files Patch % Lines
packages/frontend/src/scripts/collapsed.ts 2.41% 202 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop   #13961      +/-   ##
===========================================
- Coverage    77.84%   77.23%   -0.62%     
===========================================
  Files          185      184       -1     
  Lines        25600    25800     +200     
  Branches       487      487              
===========================================
- Hits         19929    19927       -2     
- Misses        5664     5866     +202     
  Partials         7        7              

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

tai-cha
tai-cha previously requested changes Jun 9, 2024
Copy link
Member

@tai-cha tai-cha left a comment

Choose a reason for hiding this comment

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

eslintのコーディング規約チェック通ってないですね

misskey/packages/frontend/src/components/MkNote.vue
Error:   234:4  error  Missing semicolon  semi

misskey/misskey/packages/frontend/src/components/MkSubNoteContent.vue
Error:   55:2  error  'onMounted' is not defined  no-undef
Error:   60:4  error  Missing semicolon           semi

/misskey/packages/frontend/src/pages/settings/general.vue
Error:   101:43  error  Attribute "v-if" should go before "v-model"  vue/attributes-order

Error: 55:2 error 'onMounted' is not defined no-undefが出ているとなると手元でも正常に動作してないと思われるのですがPR提出前にこちら動作確認されていますか?

packages/frontend/src/components/MkNote.vue Outdated Show resolved Hide resolved
packages/frontend/src/components/MkSubNoteContent.vue Outdated Show resolved Hide resolved
packages/frontend/src/components/MkSubNoteContent.vue Outdated Show resolved Hide resolved
packages/frontend/src/pages/settings/general.vue Outdated Show resolved Hide resolved
@FineArchs
Copy link
Contributor Author

FineArchs commented Jun 9, 2024

eslintのコーディング規約チェック通ってないですね

修正ありがとうございました。

Error: 55:2 error 'onMounted' is not defined no-undefが出ているとなると手元でも正常に動作してないと思われるのですがPR提出前にこちら動作確認されていますか?

一応動作確認はしましたが、畳み条件を「実際の表示サイズに基づく」に設定した際の被引用ノートの表示の確認に漏れがあったかもしれません。改めて確認しました。

@tai-cha tai-cha dismissed their stale review June 9, 2024 23:00

sufficiently modified

@kakkokari-gtyih
Copy link
Contributor

仮に新方式2種を実装するとなると、今度は逆に旧式を残す意味があまり感じられない気がする

@FineArchs
Copy link
Contributor Author

@kakkokari-gtyih
旧式を残すのは経過措置です。新方式2つにバグがあった時などのために念の為
将来的には消してもいいと思います。

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.

None yet

3 participants