-
Notifications
You must be signed in to change notification settings - Fork 1
Fix/ replace hamburger menu with dialog element #379
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
Conversation
✅ Deploy Preview for vuefes-2024 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
@KannoStanfoot @jiyuujin |
たしかに気持ちちょっと早く閉じているようには見えますが、
👍🏻(承知しました |
@totocalcio 不要な理由
Tips※今回の実装はここまで行う必要ないと思います。ハンバーガーメニューのa11yを行う際、展開後どのようにユーザーがそのメニューを閉じるかも合わせて考慮すると丁寧なので知見共有します。 ハンバーガーメニューを閉じさせる設計パターン
|
<dialog | ||
id="navigation-mobile-menu-trigger" | ||
ref="dialogRef" | ||
aria-label="ハンバーガーメニュー" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
aria-label はなくて良いと思います。
以下に詳細記載しました。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@KannoStanfoot @jiyuujin
レビューとTipsの共有ありがとうございました。
非常に参考になりました。今後ぜひ活用させて頂きます。
今回はaria-labelの削除のみ対応致しましたので、再度レビューお願い致します。
またNetlifyのデプロイがブロックされており、ブラウザ確認ができておりません。
一部CSSを修正したため、クロスブラウザで確認したいので、
プレビュー環境へ反映後チェックします。
- update transition property.
ひとまず netlify のプレビュー環境については無視で良いです mm cc: @totocalcio |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
🔗 Linked issue
https://github.com/vuejs-jp/vuefes-2024-backside/issues/368
📚 Description
📝 Note
autofocus
はdialog要素にするか悩んだ末、直前のボタン操作でメニューを開くことが理解できるため、リスト1項目目に設定