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: Show modal when you delete plugin #7875

Merged
merged 28 commits into from
Aug 22, 2023

Conversation

soumaeda
Copy link
Contributor

@soumaeda soumaeda commented Jul 12, 2023

https://redmine.weseek.co.jp/issues/125483
プラグインカードの削除ボタンを押すと、削除確認用のモーダルが表示されるようにしました

デザインについてもui/uxチャンネルで取り決めました
https://wsgrowi.slack.com/archives/C0111HG81GB/p1689216931459769

2023-07-13.15.53.27.mov

@soumaeda soumaeda changed the base branch from imprv/111416-create-plugin-delete-modal to master July 13, 2023 04:03
@soumaeda soumaeda temporarily deployed to VRT July 13, 2023 04:19 — with GitHub Actions Inactive
@soumaeda soumaeda temporarily deployed to VRT July 13, 2023 04:56 — with GitHub Actions Inactive
@soumaeda soumaeda temporarily deployed to VRT July 13, 2023 05:22 — with GitHub Actions Inactive
@soumaeda soumaeda temporarily deployed to VRT July 13, 2023 06:57 — with GitHub Actions Inactive
@soumaeda soumaeda temporarily deployed to VRT July 13, 2023 07:15 — with GitHub Actions Inactive
@soumaeda soumaeda temporarily deployed to VRT July 14, 2023 07:19 — with GitHub Actions Inactive
@soumaeda soumaeda temporarily deployed to VRT July 18, 2023 05:37 — with GitHub Actions Inactive
@soumaeda soumaeda temporarily deployed to VRT July 18, 2023 06:30 — with GitHub Actions Inactive
@soumaeda soumaeda temporarily deployed to VRT August 4, 2023 04:24 — with GitHub Actions Inactive
@soumaeda soumaeda temporarily deployed to VRT August 4, 2023 06:20 — with GitHub Actions Inactive
@soumaeda soumaeda temporarily deployed to VRT August 4, 2023 08:33 — with GitHub Actions Inactive
apps/app/src/components/Layout/AdminLayout.tsx Outdated Show resolved Hide resolved
apps/app/src/stores/modal.tsx Outdated Show resolved Hide resolved
apps/app/src/stores/modal.tsx Outdated Show resolved Hide resolved
apps/app/src/stores/modal.tsx Outdated Show resolved Hide resolved
@soumaeda soumaeda temporarily deployed to VRT August 7, 2023 10:02 — with GitHub Actions Inactive
@soumaeda soumaeda temporarily deployed to VRT August 8, 2023 02:16 — with GitHub Actions Inactive
@soumaeda soumaeda temporarily deployed to VRT August 8, 2023 02:35 — with GitHub Actions Inactive
Comment on lines 755 to 758
type PluginDeleteModalUtils = {
open(plugin: IGrowiPluginHasId): void,
close(): void,
}
Copy link
Contributor

Choose a reason for hiding this comment

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

戻り値の型が void でいいのか確認お願いします 🙏

Copy link
Contributor Author

Choose a reason for hiding this comment

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

voidの場合戻り値がなく、呼び出し元の処理の完了を待たずに、open、closeメソッドを実行する。
type PluginDeleteModalUtils = { open(plugin: IGrowiPluginHasId): Promise<PluginDeleteModalStatus | undefined>, close(): Promise<PluginDeleteModalStatus | undefined>, }
このように修正することで、PluginDeleteModal内の処理(await apiv3Delete(reqUrl);)が完了するまで待つことができるようになりました。

@soumaeda soumaeda temporarily deployed to VRT August 8, 2023 05:01 — with GitHub Actions Inactive
@soumaeda soumaeda temporarily deployed to VRT August 8, 2023 06:21 — with GitHub Actions Inactive
@soumaeda soumaeda temporarily deployed to VRT August 8, 2023 07:41 — with GitHub Actions Inactive
@soumaeda
Copy link
Contributor Author

soumaeda commented Aug 8, 2023

async(plugin: IGrowiPluginHasId): Promise<PluginDeleteModalStatus | undefined>

型推論で足りるところをわざわざ型アサーションおり、不要のため削除しました。

async関数内でswrResponse.Dataをreturnしていましたが、グローバルステートの更新には不要なので削除し、型エイリアスではPromise<void>と定義しました。

@@ -47,6 +49,7 @@ const AdminLayout = ({
</div>

<PageCreateModal />
<PluginDeleteModal />
Copy link
Member

Choose a reason for hiding this comment

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

ここには入れず、PluginsExtensionPageContents.tsx に入れてください

open,
close,
};
};
Copy link
Member

Choose a reason for hiding this comment

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

これも features/growi-plugin/client の中にあった方がいいね

Copy link
Contributor Author

Choose a reason for hiding this comment

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

/growi-plugin/client/stores/admin-plugins.tsxに記述しました

catch (err) {
toastError(err);
}
}, [id, closePluginDeleteModal, t, mutate]);
Copy link
Member

Choose a reason for hiding this comment

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

更にコード品質を上げるために、いくつか後続ストーリーを作ってください。

  • apiv3Delete をカスタムフック化する
    • client/services/page-operation.ts が参考になる
  • toast 表示および Modal の close を PluginsExtensionPageContents にやらせる
    • 責務の観点から

@soumaeda soumaeda temporarily deployed to VRT August 21, 2023 02:45 — with GitHub Actions Inactive
@reg-suit
Copy link

reg-suit bot commented Aug 21, 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.

@soumaeda
Copy link
Contributor Author

@jam411 jam411 merged commit cf2025e into master Aug 22, 2023
@jam411 jam411 deleted the imprv/111416-125483-show-modal-when-click-button branch August 22, 2023 00:41
@github-actions github-actions bot mentioned this pull request Aug 22, 2023
@github-actions github-actions bot mentioned this pull request Sep 4, 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