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

imprv: Limit the file types in editor. #8146

Merged
merged 17 commits into from
Oct 20, 2023

Conversation

reiji-h
Copy link
Contributor

@reiji-h reiji-h commented Oct 6, 2023

概要

  • エディタからアップロード可能なファイルに制限を設ける
  • オーバーレイは未実装
  • 制限は GROWI 起動時の環境変数で次の三つ二つから指定可能。
    • アップロードできる
    • アップロードできない
  • GROWI 内から次の二つを選択可能
    • すべてのファイルをアップロードできる
    • 画像だけアップロードできる

Task

実装

追加点は

  • ファイル制限の追加
  • ファイル制限があった時に、D&D の挙動を変える
  • ファイル制限があった時に、ツールバーの + からの Attachment ボタンの表示/非表示

今回、ファイル制限のために packages/editor/src/constsAcceptedUploadFileType を追加。CodeMirrorEditorMain がそれを受け取り、dropzone の許容するファイルタイプと Attachment ボタンに反映する。react-dropzone のファイルの制限は次を参照。

@reiji-h reiji-h temporarily deployed to VRT October 6, 2023 10:00 — with GitHub Actions Inactive
Copy link
Contributor

@jam411 jam411 left a comment

Choose a reason for hiding this comment

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

コメントエディタにもファイルをアタッチできるので、CodeMirrorEditorComment.tsx で呼び出している CodeMirrorEditor コンポーネントにも acceptedFileType を渡したい.
そして props の型をオプショナルにしなくてもよくなると思う。

Comment on lines 362 to 370
const acceptedFileType = useMemo(() => {
if (isUploadableFile) {
return AcceptedUploadFileType.ALL;
}
if (isUploadableImage) {
return AcceptedUploadFileType.IMAGE;
}
return AcceptedUploadFileType.NONE;
}, [isUploadableFile, isUploadableImage]);
Copy link
Contributor

Choose a reason for hiding this comment

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

権限として環境変数によるファイルアップロードの有効化が最も強いので、isUpladableImage の状態によらず、 isUploadableFile が false の時は return AcceptedUploadFileType.NONE; を返したい

Copy link
Contributor Author

@reiji-h reiji-h Oct 20, 2023

Choose a reason for hiding this comment

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

確かに今のままだと、画像のみアップロード可能にするためにファイルのアップロードを禁止にする必要がある変な状況になっていますね…。理想の挙動をするように変更しました。

const accept: Accept = {};
accept[(acceptedFileType ?? '')] = [];

const noDrag = acceptedFileType == null;
Copy link
Contributor

Choose a reason for hiding this comment

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

定数を使えばいいと思う。
const noDrag = acceptedFileType === AcceptedUploadFileType.NULL

undefined の場合も考慮していたらすまん

Copy link
Contributor Author

@reiji-h reiji-h Oct 20, 2023

Choose a reason for hiding this comment

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

定数を使う形で変更しました。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

noDrag だとまだ Dropzone を使えてしまうような書き方なので、 disabled を使うように変更しました。

Comment on lines 24 to 25
const accept: Accept = {};
accept[(acceptedFileType ?? '')] = [];
Copy link
Contributor

Choose a reason for hiding this comment

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

  • オプショナルにする必要がなければこのあたりもシンプルになる想定

Copy link
Contributor Author

@reiji-h reiji-h Oct 20, 2023

Choose a reason for hiding this comment

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

オプショナルを外す形に変更しました。

Copy link
Contributor

@jam411 jam411 left a comment

Choose a reason for hiding this comment

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

開発環境で動きますかね?以下のエラーがでます。

Error: [vite]: Rollup failed to resolve import "src/consts" from "/workspace/growi/packages/editor/src/services/file-dropzone/use-file-dropzone.ts".
@growi/editor:dev: This is most likely unintended because it can break your application at runtime.
@growi/editor:dev: If you do want to externalize this module explicitly add it to
@growi/editor:dev: `build.rollupOptions.external`

Comment on lines 24 to 25
const accept: Accept = {};
accept[(acceptedFileType)] = [];
Copy link
Contributor

Choose a reason for hiding this comment

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

これできないか?

const accept: Accept = {
  acceptedFileType: []
};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

出来たのでそのように変更しました。
エラーについては import に使用するパスがおかしかったので修正しました。

@reiji-h reiji-h temporarily deployed to VRT October 20, 2023 09:10 — with GitHub Actions Inactive
@reg-suit
Copy link

reg-suit bot commented Oct 20, 2023

reg-suit detected visual differences.

Check this report, and review them.

🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴
⚪⚪⚪
⚫⚫
🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵

What do the circles mean? The number of circles represent the number of changed images.
🔴 : Changed items, ⚪ : New items, ⚫ : Deleted items, and 🔵 Passed items

How can I change the check status? If reviewers approve this PR, the reg context status will be green automatically.

ALL: '*',
IMAGE: 'image/*',
NONE: '',
} as const;
Copy link
Member

Choose a reason for hiding this comment

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

細かいんだけど、今 app 側でこの const をインポートしている。ということは中身の * っていう文字列を、app 側も取り扱っていることになるよね(見えていないだけで)。

しかし dropzone がその形式の文字列を必要としているだけで、accept を設定する直前で手に入れば app 側は不要。つまり packages/editor に閉じたまま取り扱える文字列。

その点が少し気持ち悪いなあと思っているが…どうしてお直さないといけないようなものでもないかなあとか…

@yuki-takei yuki-takei merged commit 55ab711 into dev/7.0.x Oct 20, 2023
@yuki-takei yuki-takei deleted the imprv/131762-131770-limit-file-type branch October 20, 2023 11:32
@github-actions github-actions bot mentioned this pull request Oct 20, 2023
@yuki-takei yuki-takei mentioned this pull request Mar 26, 2024
@github-actions github-actions bot mentioned this pull request Mar 26, 2024
@github-actions github-actions bot mentioned this pull request Oct 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants