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.

Sign up for GitHub

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

Jump to bottom

AppHeaderのページを追加 #1472

Merged
merged 17 commits into from
Dec 27, 2024
Merged

AppHeaderのページを追加 #1472

merged 17 commits into from
Dec 27, 2024
+236 −128

Conversation

tosiiu
Copy link
Contributor

@tosiiu tosiiu commented Dec 26, 2024 •
edited
Loading

課題・背景

  • AppHeaderのコンポーネントが新たに生まれたのでその対応
  • モバイル環境でのヘッダーの扱い方が揺れているため基準として明文化

やったこと

  • AppHeaderの追加
    • 概ねデザインパターン「ヘッダー」の内容を引き継ぎつつ、AppHeaderでは意識しなくて良い記載は削除しています。
    • モバイル環境でのレイアウトについて記載
  • デザインパターン「ヘッダー」の削除
  • デザインパターン「ヘッダー」へのリンクの置き換え
  • デザインパターン「ヘッダー」からAppHeaderへのリダイレクト

やらなかったこと

動作確認

https://deploy-preview-1472--smarthr-design-system.netlify.app/products/components/app-header/

キャプチャ

Before After

@netlify Netlify
Copy link

netlify bot commented Dec 26, 2024 •
edited
Loading

✅ Deploy Preview for smarthr-design-system ready!

Name Link
🔨 Latest commit 549b5a8
🔍 Latest deploy log https://app.netlify.com/sites/smarthr-design-system/deploys/676e2c59aa0cb90008b0bf36
😎 Deploy Preview https://deploy-preview-1472--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.

@tosiiu tosiiu marked this pull request as ready for review December 26, 2024 05:02
@tosiiu tosiiu requested a review from a team as a code owner December 26, 2024 05:02
versionfive
versionfive reviewed Dec 26, 2024
View reviewed changes
Copy link
Contributor

@versionfive versionfive left a comment •
edited
Loading

Choose a reason for hiding this comment

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

AppHeaerのREADMEもあるので、こちらとも整合がとれるとよさそうです。
https://github.com/kufu/smarthr-ui/blob/v62.3.1/packages/smarthr-ui/src/components/AppHeader/README.md

コンポーネントの説明ドキュメントなので、各要素の説明に対応するpropsを示せると便利そう。

src/content/articles/products/components/app-header/index.mdx Outdated Show resolved Hide resolved
src/content/articles/products/components/app-header/index.mdx Outdated Show resolved Hide resolved
src/content/articles/products/components/app-header/index.mdx Outdated
Comment on lines 16 to 17
- A. [グローバルヘッダー（上）](#h3-0)
- B. [アプリナビゲーション（下）](#h3-1)
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

Choose a reason for hiding this comment

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

デザインパターンのヘッダーにあったものを転機してきた形なんですが、特にこの名称でコミュニケーションすることない気がするのでHeader/AppNaviの方がシンプルかもしれませんね〜

Copy link
Contributor Author

Choose a reason for hiding this comment

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

f5203e0 でHeader/AppNaviに変えてみました

src/content/articles/products/components/app-header/index.mdx Outdated Show resolved Hide resolved
wentzzz
wentzzz reviewed Dec 26, 2024
View reviewed changes
Copy link
Contributor

@wentzzz wentzzz left a comment

Choose a reason for hiding this comment

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

コメントしました！

src/content/articles/products/components/app-header/index.mdx Outdated

![開いた状態のアプリランチャー](./images/opened-app-launcher.png)

#### A-4. スクールへのリンク
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

Choose a reason for hiding this comment

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

なるほど。今のAppHeaderの実装ではスクールをHeaderに表示するpropsが用意されているので、実装にあたってアカウントメニューの中に入れる方針が抜けてそうですかね。
別途実装の変更を検討出来ると良さそうかなと思うので、一旦基準からはスクールを除外します。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ac61197 で対応しました！

src/content/articles/products/components/app-header/index.mdx Outdated Show resolved Hide resolved
src/content/articles/products/components/app-header/index.mdx Outdated

![開いた状態のユーザーアカウントボタン](./images/opened-user-account-button-mobile.png)

#### A-4. ハンバーガーボタン
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

Choose a reason for hiding this comment

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

f5203e0 で変更してみました

versionfive
versionfive reviewed Dec 27, 2024
View reviewed changes
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.

追加でコメントしました。
モバイルの画像がちょっとでかいので幅調整したいみがありますね。

src/content/articles/products/components/app-header/index.mdx Outdated Show resolved Hide resolved
src/content/articles/products/components/app-header/index.mdx Outdated Show resolved Hide resolved
src/content/articles/products/components/app-header/index.mdx Outdated Show resolved Hide resolved
@tosiiu tosiiu requested a review from versionfive December 27, 2024 04:00
versionfive
versionfive approved these changes Dec 27, 2024
View reviewed changes
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!

wentzzz
wentzzz approved these changes Dec 27, 2024
View reviewed changes
Copy link
Contributor

@wentzzz wentzzz left a comment

Choose a reason for hiding this comment

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

gogo

@tosiiu tosiiu merged commit 2e3a340 into main Dec 27, 2024
5 checks passed
@tosiiu tosiiu deleted the add-appheader branch December 27, 2024 05:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@versionfive versionfive versionfive approved these changes

@wentzzz wentzzz wentzzz approved these changes

@oremega oremega Awaiting requested review from oremega

@ayumizu ayumizu Awaiting requested review from ayumizu

@fujitha fujitha Awaiting requested review from fujitha

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@tosiiu @wentzzz @versionfive