-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
feat(frontend): ノート・ユーザータイムライン埋め込み #13929
base: develop
Are you sure you want to change the base?
Conversation
このPRによるapi.jsonの差分 差分はこちら |
/preview |
考えられる対応
|
ページ遷移は必ず新しいタブ の方が良いわね |
あと何らかの原因で「読み込みに失敗しました」ページが表示されてしまうとそこからいろいろアクセスできてしまいそう |
あとWebSocketに繋ぐのは無駄そう |
将来的にLTL埋め込みとかが入ると使うかもしれない(ただし通常の埋め込みに必要ないのは事実なのでBootOptionsで切れるようにするのがよさそう |
LTL埋め込む場合でもポーリングで事足りそう |
navHookで全部別窓に飛ばすようにした |
これってどこ由来の画面だっけ |
サーバー側のbootも分けた方が良さそう |
セキュリティやパフォーマンスを考慮して分けられるところは極力分けたい |
今そういう実装になってるかまだ確認できてないけど、↑と同様の理由でクライアントからAPIリクエストを発生させないような実装にしたい |
半SSRみたいな感じ |
フロントエンドを使い回す意味が殆どなくなってしまう(結局 #10491 になりそう) あと起動処理やstoreは分離していてアカウントはnullを返すようになってるので問題はなさそうではある |
/preview |
@@ -645,6 +648,8 @@ export function popupMenu(items: MenuItem[], src?: HTMLElement | EventTarget | n | |||
} | |||
|
|||
export function contextMenu(items: MenuItem[], ev: MouseEvent): Promise<void> { | |||
if (embedPage) return Promise.resolve(); |
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.
これはなぜかしら
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.
popupを全面的に無効にしているのと、iframe中で別のコンテキストメニューがでると混乱を招く可能性があるので
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.
popup全面的に無効ってなんでだっけ
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.
意図せずModalが立ち上がったりするのを防ぐため(主にlogin系)
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.
うーむ
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.
そもそもpopupを無効化するのがなんかよくわからない
うーむ
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.
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.
それはpopup呼び出す側で制御するべきそう
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.
それはpopup呼び出す側で制御するべきそう
制御を組み込む箇所が多岐にわたる+embed側で読み込みを許容するpopupが少ない
となると、読み込みの可否を制御するロジックをpopupが呼ばれる個所一つ一つに入れるのは現実的ではないなと思ってpopupのオプションにてオプトインで埋め込みページでも表示できるような仕組みにしておいたけどそれじゃ不味いかしら
const { dispose } = popup(SomeComponent, {
propsKey: 'foo',
}, {
closed: () => dispose(),
}, {
+ callEvenOnEmbedPage: true,
});
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.
↑というかこれはpopup呼ぶ側で制御できることになるのでは?(埋め込みページデフォルトOFF、オプションでONにできるので)
done |
コンフリクト解消 |
What
Why
Fix #1714
Close #10491
Additional info (optional)
カスタマイズ用 パラメータ
共通
colorMode
:light
|dark
|auto
。カラーモードのオーバーライド。デフォルト:auto
rounded
:true
|false
。角丸にするかどうか。デフォルト:true
border
:true
|false
。外周のボーダーをつけるかどうか。デフォルト:true
TL系
header
:true
|false
。上部にヘッダを設けるかどうか。デフォルト:true
autoload
:true
|false
。無限スクロールするかどうか。デフォルト:false
maxHeight
: 数値。設定すると、指定した高さ以降はスクロールバーが出る。指定しない場合はどこまでも伸びる(デフォルトで何らかの値を設定した状態のコードを生成する)埋め込みコード
たぶんミニファイして2行になると思う
その他
/packages/backend/assets/embed.js
(親ページ側から読み込むiframe制御用スクリプト)はいろいろ使いまわしやすいようにSPDXをMITライセンスとしているChecklist