Skip to content

fix: focus styles#908

Merged
cooper-joe merged 8 commits intomasterfrom
focus-stresstest
Dec 20, 2021
Merged

fix: focus styles#908
cooper-joe merged 8 commits intomasterfrom
focus-stresstest

Conversation

@cooper-joe
Copy link
Member

@cooper-joe cooper-joe commented Dec 14, 2021

Fixes UX-20.

This PR fixes focus styles across the library. Focus styles were previously updated, but there were issues where the focus outline was cut off by the parent container. All focus styles are now contained within the element.

For example, issues like:
image


Screenshots

Component Before After
Button image image
Button secondary image image
Button primary image image
Button destructive image image
Checkbox image image
Radio image image
Input / Select image image
Segmented control image image
Switch image image

Note: The focus styles appear outside of the Checkbox, Radio, and Switch, but are actually inside the bounds of the element.

@dhis2-bot
Copy link
Contributor

dhis2-bot commented Dec 14, 2021

🚀 Deployed on https://pr-908--dhis2-ui.netlify.app

@dhis2-bot dhis2-bot temporarily deployed to netlify December 14, 2021 12:20 Inactive
@cypress
Copy link

cypress bot commented Dec 14, 2021



Test summary

555 0 0 0


Run details

Project ui
Status Passed
Commit 0e2f414
Started Dec 14, 2021 2:33 PM
Ended Dec 14, 2021 2:41 PM
Duration 08:04 💡
OS Linux Ubuntu - 20.04
Browser Electron 89

View run in Cypress Dashboard ➡️


This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

@dhis2-bot dhis2-bot temporarily deployed to netlify December 14, 2021 14:29 Inactive
@cooper-joe cooper-joe requested a review from a team December 14, 2021 14:49
@cooper-joe cooper-joe marked this pull request as ready for review December 14, 2021 14:49
@mediremi mediremi self-requested a review December 14, 2021 15:00
textarea:focus {
outline: none;
box-shadow: 0 0 0 3px ${theme.focus};
box-shadow: inset 0 0 0 2px ${theme.focus};
Copy link

Choose a reason for hiding this comment

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

Should the spread radius here (the 2px) not be 3px?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is correct, he's using an inset box-shadow with a width of 2px plus a blue border of 1px, so the total visual width is 3px.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's right, the box-shadow and border combine to make 3px.

Copy link

Choose a reason for hiding this comment

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

Ah nice. Yeah I assumed it was intentional since it was changed from 3 to 2, but it stood out to me so I thought I'd ask.

@cooper-joe cooper-joe merged commit b2a53aa into master Dec 20, 2021
@cooper-joe cooper-joe deleted the focus-stresstest branch December 20, 2021 08:49
@dhis2-bot
Copy link
Contributor

🎉 This PR is included in version 7.10.5 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants