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

ErrorScreenの使用上の注意を追加する #1164

Merged
merged 62 commits into from
Jun 18, 2024
Merged

Conversation

oti
Copy link
Contributor

@oti oti commented May 21, 2024

課題・背景

https://smarthr.atlassian.net/browse/SD-654

基準充足のため

やったこと

ErrorScreenの使用上の注意を追加した。

やらなかったこと

Storybook のパターンが謎で、修正するとなっても別リポジトリなのでいったんそのままにしています。

  • Full:何がフルなのかわからない(画面が?幅が?高さが?propsが?)
  • WithoutChildren: children props なしでも動作するよ、というコンポーネントの仕様を語ってそうだけどわからない
  • WithFooter:footer props に ReactNode を入れられるよという話をしているようだけど、各プロダクトを検索した感じでは ErrorScreen でフッターを表示しているものが見受けられない。グループ企業にはあるかも?
  • SmarthHR UI側の対応はfooter props をOmitするPRをマージ済み
  • Storybookも無味無臭なものに変えてあります

動作確認

https://deploy-preview-1164--smarthr-design-system.netlify.app/products/components/error-screen/

キャプチャ

Before After

@oti oti requested review from versionfive, tosiiu and a team as code owners May 21, 2024 07:38
@oti oti requested review from Qs-F and ytysym May 21, 2024 07:38
Copy link

netlify bot commented May 21, 2024

Deploy Preview for smarthr-design-system ready!

Name Link
🔨 Latest commit 15d1a03
🔍 Latest deploy log https://app.netlify.com/sites/smarthr-design-system/deploys/66710aed231d110008513130
😎 Deploy Preview https://deploy-preview-1164--smarthr-design-system.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Contributor

@tosiiu tosiiu left a comment

Choose a reason for hiding this comment

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

現状一貫性が無いそれぞれのpropsの扱い方(logoを有効化する/しない, titleにHTTPステータスコードを入れる/入れない, childrenに渡すテキストの内容など)の基準がほしいと思いました!

Copy link
Contributor

@versionfive versionfive left a comment

Choose a reason for hiding this comment

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

ura sanと同様に挿入するメッセージのバリエーションがほしいと思いました。(メンテナンス中以外でも40x系や50x系のエラー画面でつかわれているため)

@oti oti changed the title ErrorScreenの使用目的を追加した ErrorScreenの使用上の注意を追加した Jun 10, 2024
@oti oti requested a review from tosiiu June 10, 2024 09:01
@oti oti requested review from INABAshina and Qs-F June 13, 2024 10:21
@oti
Copy link
Contributor Author

oti commented Jun 13, 2024

@INABAshina 提案ありがとうございました! どれも納得のものだったので取り込みました!

みなさま、改めてレビューをお願いいたします! @Qs-F @tosiiu @versionfive

Copy link
Contributor

@INABAshina INABAshina left a comment

Choose a reason for hiding this comment

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

@oti 1点だけ追加で提案とコメントを入れています🙏><
ほかはLGTMでした!

Qs-F
Qs-F previously approved these changes Jun 14, 2024
Copy link
Contributor

@Qs-F Qs-F left a comment

Choose a reason for hiding this comment

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

個人的には引っかかりポイントがなく、するっと理解できて良い気がしたのでLGTMです!

Copy link
Contributor

@versionfive versionfive left a comment

Choose a reason for hiding this comment

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

1点質問とSuggestionをいれております。文言が整ってありがたい…

Copy link
Contributor

@tosiiu tosiiu left a comment

Choose a reason for hiding this comment

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

既出コメント以外LGTMです!

@oti
Copy link
Contributor Author

oti commented Jun 18, 2024

@INABAshina @tosiiu @versionfive @Qs-F 対応漏れなどを作業したらapproveがdismissになっちゃいました。繰り返しで大変恐縮ですが再度確認をお願いいたします🙇

@oti
Copy link
Contributor Author

oti commented Jun 18, 2024

補足:SmartHR UI側でErrorScreenのレイアウトを修正する(上下左右の余白、中央揃え)

Copy link
Contributor

@versionfive versionfive left a comment

Choose a reason for hiding this comment

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

#1164 (comment)
の前提を確認しました。
1点だけ、フィードバックが生まれたのでそちらにリンクを追加しました。


具体例は<a href="#h2-2">エラー別の表示内容</a>に記述しています。

フォームのバリデーションエラーや連携APIの疎通エラーのような一時的なエラーの場合、[NotificationBar](/products/components/notification-bar/)や[ResponseMessage](/products/components/response-message/)、[InformationPanel](/products/components/information-panel/)を使用を検討してください。
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
フォームのバリデーションエラーや連携APIの疎通エラーのような一時的なエラーの場合、[NotificationBar](/products/components/notification-bar/)[ResponseMessage](/products/components/response-message/)[InformationPanel](/products/components/information-panel/)を使用を検討してください
フォームのバリデーションエラーや連携APIの疎通エラーのような一時的なエラーの場合、[NotificationBar](/products/components/notification-bar/)[ResponseMessage](/products/components/response-message/)[InformationPanel](/products/components/information-panel/)などを使用を検討します。詳細は[フィードバック](/products/design-patterns/feedback/)の基準を参照してください

フィードバックに誘導

@oti oti requested a review from versionfive June 18, 2024 04:20
@oti
Copy link
Contributor Author

oti commented Jun 18, 2024

@versionfive フィードバックへの動線を追加しました!

Copy link
Contributor

@versionfive versionfive left a comment

Choose a reason for hiding this comment

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

LGTM!!!明快になってすぐ使える基準が爆誕しましたね!

Copy link
Contributor

@Qs-F Qs-F left a comment

Choose a reason for hiding this comment

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

読みやすかったです!! 👏

Copy link
Contributor

@tosiiu tosiiu left a comment

Choose a reason for hiding this comment

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

LGTM!

@oti oti merged commit eb8d41b into main Jun 18, 2024
5 checks passed
@oti oti deleted the add-guideline-ErrorScreen branch June 18, 2024 07:07
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.

5 participants