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: add aria-label to MessageScreen #1018

Merged
merged 3 commits into from
Oct 5, 2020
Merged

fix: add aria-label to MessageScreen #1018

merged 3 commits into from
Oct 5, 2020

Conversation

cidermitaina
Copy link
Member

@cidermitaina cidermitaina commented Sep 24, 2020

Related URL

https://smarthr.atlassian.net/browse/SHRUI-152

Overview

MessageScreen--fulltarget=_blankなリンクが達成基準 1.1.1 非テキストコンテンツに非適合なのでaria-labelを追加しました。

What I did

  • MessageScreen--fulltarget=_blankリンクにaria-label="別タブで開く"を追加

Capture

image

@cidermitaina cidermitaina requested a review from a team as a code owner September 24, 2020 04:36
@cidermitaina cidermitaina requested review from nabeliwo and Tokky0425 and removed request for a team September 24, 2020 04:36
@netlify
Copy link

netlify bot commented Sep 24, 2020

Deploy preview for smarthr-ui ready!

Built with commit 9d8d92a

https://deploy-preview-1018--smarthr-ui.netlify.app

@reg-suit
Copy link

reg-suit bot commented Sep 24, 2020

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

Check out the report here.

@cidermitaina cidermitaina changed the title [WIP] fix: add aria-label to MessageScreen fix: add aria-label to MessageScreen Sep 24, 2020
@cidermitaina
Copy link
Member Author

@nabeliwo @Tokky0425
Please review!

@cidermitaina cidermitaina self-assigned this Sep 24, 2020
Copy link
Member

@nabeliwo nabeliwo left a comment

Choose a reason for hiding this comment

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

対応ありがとうございます!コメントしたのでご確認をよろしくお願いします 🙏

@@ -39,7 +39,7 @@ export const MessageScreen: FC<Props> = ({ title, links, children, className = '
<li key={link.label}>
<Link
href={link.url}
{...(link.target ? { target: link.target } : {})}
{...(link.target ? { target: link.target, 'aria-label': '別タブで開く' } : {})}
Copy link
Member

Choose a reason for hiding this comment

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

[MUST] PR の説明を見ると「target=_blank の時に 'aria-label': '別タブで開く' を追加する」という条件のようですが、現状だと「target が存在した場合 target の値とは関係なく 'aria-label': '別タブで開く' を追加する」という記述になっているようです。

Copy link
Member Author

Choose a reason for hiding this comment

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

ほんとだ…!🙇‍♀️

こちらで修正しましたのでご確認お願いします!
5e4d235

@cidermitaina
Copy link
Member Author

@nabeliwo
ご指摘箇所修正しましたので、お手すきの際見ていただけると嬉しいです🙋‍♀️

Copy link
Member

@nabeliwo nabeliwo left a comment

Choose a reason for hiding this comment

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

LGTM 🎉

@cidermitaina cidermitaina merged commit ef56e37 into master Oct 5, 2020
@cidermitaina cidermitaina deleted the SHRUI-152 branch October 5, 2020 05:23
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.

None yet

2 participants