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

fix(spindle-ui): remove closing className #595

Merged
merged 1 commit into from Dec 16, 2022
Merged

fix(spindle-ui): remove closing className #595

merged 1 commit into from Dec 16, 2022

Conversation

lanberb
Copy link
Contributor

@lanberb lanberb commented Nov 25, 2022

概要

AppealModalコンポーネントをEscキーで閉じた後、再度開いた際に正常に表示されない挙動を修正しました。

原因

  • .spui-spui-AppealModal--closingがModalを閉じた後も残ったままだった。
    • 同classNameの付与を担うclosingフラグが更新されていなかった。

修正内容

  • dialog要素のonCloseにclosingを更新するsetClosing(false)を渡し、Modalを閉じた後に必ずclosing === falseになるようにしました。

@lanberb lanberb self-assigned this Nov 25, 2022
@lanberb lanberb removed the request for review from herablog November 25, 2022 05:59
@lanberb lanberb changed the title fix(spindle-ui): remove closing className [WIP] fix(spindle-ui): remove closing className Nov 25, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Nov 25, 2022

Visit the preview URL for this PR (updated for commit 3101474):

https://ameba-spindle--pr595-fix-appeal-modal-4wv3aoml.web.app

(expires Sun, 15 Jan 2023 01:04:47 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Sign: e7521619a2dd5c653490c8246e81ec2a5c8f1435

@reg-suit
Copy link

reg-suit bot commented Nov 25, 2022

✨✨ That's perfect, there is no visual difference! ✨✨

Check out the report here.

@lanberb lanberb changed the title [WIP] fix(spindle-ui): remove closing className fix(spindle-ui): remove closing className Nov 25, 2022
@lanberb
Copy link
Contributor Author

lanberb commented Nov 25, 2022

testが落ちていますが、es-lintのissueでbugとして報告されていました。
jsx-eslint/eslint-plugin-react#3436

こちらのPR(#530 )のmergeで解消される見込みです。

@lanberb lanberb requested review from itsminadesu and removed request for itsminadesu November 25, 2022 06:37
@herablog
Copy link
Member

testが落ちていますが、es-lintのissueでbugとして報告されていました。 jsx-eslint/eslint-plugin-react#3436

こちらのPR(#530 )のmergeで解消される見込みです。

お、そしたらそちら先に確認してマージしてもらちゃってよいでしょか!

@herablog
Copy link
Member

testが落ちていますが、es-lintのissueでbugとして報告されていました。 jsx-eslint/eslint-plugin-react#3436

こちらのPR(#530 )のmergeで解消される見込みです。

こりやってもらったみたいなので、いちおmainの内容このブランチに取り込んじゃってください!!

@herablog
Copy link
Member

herablog commented Dec 1, 2022

動きとても良さそうです!!

@herablog
Copy link
Member

herablog commented Dec 1, 2022

チェンジログに載る都合で、コミットメッセージを変更したことでなく、修正した内容 ( fix(spindle-ui): close dialog with escape key 的な?) に変更お願いしたいです!!

@lanberb lanberb force-pushed the fix/appeal-modal branch 3 times, most recently from d71099a to 223bf41 Compare December 2, 2022 01:42
@lanberb
Copy link
Contributor Author

lanberb commented Dec 2, 2022

@herablog
close dialog with escape keyは元々挙動としてはできていたので、fix(spindle-ui): close handler to reopen dialogに変更してみました🙇

@herablog
Copy link
Member

herablog commented Dec 2, 2022

あと Dialog も同じ問題がありそうなので、一緒に修正お願いしたいです!

@lanberb
Copy link
Contributor Author

lanberb commented Dec 16, 2022

@herablog
DialogのハンドラもAppealModalと同様に修正しました!

Copy link
Member

@herablog herablog left a comment

Choose a reason for hiding this comment

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

エンジニアを褒めるネコ:ありがとうございます

@lanberb lanberb merged commit 3949ad5 into main Dec 16, 2022
@lanberb lanberb deleted the fix/appeal-modal branch December 16, 2022 01:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants