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

Implement warning dialog #3

Merged
merged 14 commits into from
Sep 27, 2024
Merged

Implement warning dialog #3

merged 14 commits into from
Sep 27, 2024

Conversation

HashidaTKS
Copy link
Contributor

@HashidaTKS HashidaTKS commented Sep 20, 2024

Add a warning dialog.

This patch based on #4, we should merge #4 beforehand in order to work this patch correctly.

Currently, it works only for trusted domains and untrusted domains, don't work for attachment or others.

image

The UI is implemented with Fluent UI for we components for style.

@HashidaTKS
Copy link
Contributor Author

The current implementation is "base" for the warning dialog.
We should update design and add some functions.

I think it is better to update/add them in other pull requests.
So I request review for this at current status.

@piroor

Would you review this when you have time?

@@ -1,4 +1,68 @@
let totalCheckboxCount = 0;
function parse(recipient) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

parse and RecipientClassifier are copied from .mjs files.
This is because adding

<script type="module" src="recipient-classifier.mjs"></script>
<script type="module" src="recipient-parser.mjs"></script>

to dialog.html raises an error like below.

Failed to load module script: Expected a JavaScript module script but the server responded with a MIME type of "text/plain". Strict MIME type checking is enforced for module scripts per HTML spec.

I think we should fix web server implementation so as to specify "javascript", but I think it is better to fix it in another PR in order to narrow the scope of modification.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed to use .mjs files.

We should merge #4 before this PR in order to work this correctly.

@@ -8,12 +72,27 @@ Office.onReady(function () {
sendStatusToParent("ready");
});

let counter = 0;
function generateTempId() {
return `fcm_temp_${counter++}_${Date.now()}`;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or, simply using GUID and so on.

@HashidaTKS HashidaTKS requested a review from piroor September 24, 2024 04:38
@HashidaTKS
Copy link
Contributor Author

HashidaTKS commented Sep 24, 2024

警告のアドレスの先頭にToやCcを追加しないといけないことに気付いたため、修正します。

@HashidaTKS
Copy link
Contributor Author

image

追加しました。

@HashidaTKS HashidaTKS force-pushed the implement-warning-dialog branch from 19b94c9 to 0e6a9d2 Compare September 24, 2024 08:11
Copy link
Member

@piroor piroor left a comment

Choose a reason for hiding this comment

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

気になった所へコメントしました。

src/web/dialog.html Show resolved Hide resolved
function generateTempId() {
return `fcm_temp_${counter++}_${Date.now()}`;
}

function sendStatusToParent(status) {
const messageObject = { status: status };
const jsonMessage = JSON.stringify(messageObject);
Office.context.ui.messageParent(jsonMessage);
}

// eslint-disable-next-line @typescript-eslint/no-unused-vars
Copy link
Member

Choose a reason for hiding this comment

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

これらの「eslint-disable-next-line @typescript-eslint/no-unused-vars」は、関数が定義されているのに未使用だと判断されることによるlintのエラーを出さないようにするための物なのですが、今の状態でもまだ必要そうでしょうか?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

lintでエラーにならなかったのでスルーしてしまったのですが、確かに不要そうですので、確認の上、不要であれば削除します。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

不要だったため削除しました。

};

function appendCheckboxes(target, groupedRecipients) {
for (const key in groupedRecipients) {
Copy link
Member

Choose a reason for hiding this comment

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

ここは

for (const [key, recipients] of Object.entries(groupedRecipients)) {

でいけそうに思いますが、どうでしょうか?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

for (const [key, recipients] of Object.entries(groupedRecipients)) {

を試して動かなかったような覚えがあるのですが、

for (const [key, recipients] in Object.entries(groupedRecipients)) {

としていて動かなかったのかもしれないため、確認します。

Copy link
Contributor Author

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.

修正しました。

@HashidaTKS
Copy link
Contributor Author

@piroor

確認ありがとうございます。指摘事項に対応しました。

@HashidaTKS HashidaTKS requested a review from piroor September 27, 2024 05:56
Copy link
Member

@piroor piroor left a comment

Choose a reason for hiding this comment

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

Looks good to merge.

@piroor piroor merged commit 2bdd831 into main Sep 27, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants