-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
fix: 設定とコンパネでPageMetadataが切り替わらない問題など (taiyme/misskey#186) #13530
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #13530 +/- ##
===========================================
+ Coverage 77.90% 77.93% +0.02%
===========================================
Files 185 185
Lines 25504 25531 +27
Branches 487 487
===========================================
+ Hits 19870 19897 +27
Misses 5627 5627
Partials 7 7 ☔ View full report in Codecov by Sentry. |
/preview |
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.
特段問題なさそうだと思ったのですが、フロントエンドのレビューはあまり得意ではないため他の人にもみて欲しいかも
(ところでこれdraftは外し忘れですかね?)
}); | ||
|
||
const cleanups: (() => void)[] = []; | ||
const cleanup = () => { | ||
for (const cl of cleanups) cl(); |
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.
要素数が少ないので大きな差はないし特にコーディングルールがある認識でもないので好みの問題かもしれないですが
for (const cl of cleanups) cl(); | |
cleanups.forEach(cl => cl()); |
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.
個人的にはforEachで回す方が好みですが、既にfor...ofで実装されているコードがあったので、そちらに合わせました
for (const cl of cleanups) { |
for (const cl of cleanups) { |
draft外しました! |
@supports (height: 100cqh) { | ||
height: 100cqh; | ||
overflow: hidden; // fallback (overflow: clip) | ||
overflow: clip; | ||
contain: strict; | ||
} |
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.
自分のforkの都合でCSSのフォールバックを含んでいますが、misskey-dev/misskeyの方針では不要でしょうか?
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.
これのフォールバックは入っていなかったはず
height: 100%; | ||
margin: 0 auto; | ||
display: flex; | ||
box-sizing: border-box; | ||
|
||
@supports (height: 100cqh) { | ||
height: 100cqh; | ||
overflow: hidden; // fallback (overflow: clip) | ||
overflow: clip; | ||
contain: strict; | ||
} |
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.
フォールバックが不要であれば
height: 100%; | |
margin: 0 auto; | |
display: flex; | |
box-sizing: border-box; | |
@supports (height: 100cqh) { | |
height: 100cqh; | |
overflow: hidden; // fallback (overflow: clip) | |
overflow: clip; | |
contain: strict; | |
} | |
height: 100cqh; | |
overflow: clip; | |
contain: strict; | |
margin: 0 auto; | |
display: flex; | |
box-sizing: border-box; |
/preview |
スクロールの扱い的に、クラシックUIで都合が悪そう |
コンフリクト解消 |
/preview |
コンフリクト解消 |
What
Why
resolve #13450
ただし、以下の問題について根本的な修正は実施していません。
(いずれ別のPRで。 #12836 (comment) の実装を待ちたい)
Additional info (optional)
Cherry-picked from taiyme/misskey
Note:
admin/_header_.vue
を削除し、MkPageHeaderに置き換えましたadmin/_header_.vue
で使用していたheaderActionsのasFullButtonオプションをMkPageHeaderで実装しましたChecklist