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: Prevent focus ring from getting cut off in Popup.Body #1727

Merged
merged 47 commits into from Sep 9, 2022

Conversation

mannycarrera4
Copy link
Contributor

@mannycarrera4 mannycarrera4 commented Aug 11, 2022

Summary

Fixes: #1682

category

Release Note

  • Popup.Body now has 8px of padding. This addresses an issue introduced in v7 where focus rings were being clipped within the Body due to its built-in overflow; this padding allows the focus ring on any focusable element contained within the Body to be fully visible. The default space between the edge of the Card and the contents of the Body is still 32px.
  • Popup.Heading now has 8px of padding to keep it visually aligned with the updated Popup.Body. It also now has a marginBottom of 8px, bringing the default vertical space between the Heading and the Body to 24px (previously 16px) in order to meet the component spec.
  • The padding in Popup.Card has been reduced by 8px from 32px to 24px to compensate for the padding added to Popup.Body and Popup.Heading and to preserve the component's existing spacing. If you've customized the padding for any Popup, Modal, or Dialog subcomponents, you may need to adjust your padding values to get the same results after upgrading.
  • Popup.Card's default element is now a Flex element (previously a Stack) and thus the spacing prop has been removed. It made more sense to use Flex given the presence of the Close Icon, which didn't make sense as a Stack item.
  • Given the changes above, if you're creating a footer for your Popup, Modal, or Dialog (using an HStack, for instance) you'll need to update it with the following props/styles to align it with the updated Heading and Body: padding="xxs" (8px) and marginTop="xxs" (8px).

Screen Shot 2022-09-08 at 19 49 50

@mannycarrera4 mannycarrera4 changed the base branch from master to prerelease/minor August 11, 2022 17:13
@cypress
Copy link

cypress bot commented Aug 11, 2022



Test summary

718 0 2 0Flakiness 0


Run details

Project canvas-kit
Status Passed
Commit aaf0a3c ℹ️
Started Sep 8, 2022 9:45 PM
Ended Sep 8, 2022 9:50 PM
Duration 05:33 💡
OS Linux Ubuntu - 20.04
Browser Electron 94

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

@mannycarrera4 mannycarrera4 added this to In Progress in Current Sprint (7/20 - 8/9) via automation Aug 11, 2022
@mannycarrera4 mannycarrera4 added 7.x bug Something isn't working labels Aug 11, 2022
@mannycarrera4 mannycarrera4 marked this pull request as ready for review August 12, 2022 15:07
modules/react/modal/lib/ModalFooter.tsx Outdated Show resolved Hide resolved
modules/react/modal/lib/Modal.tsx Outdated Show resolved Hide resolved
@mannycarrera4 mannycarrera4 added the ready for review Code is ready for review label Aug 15, 2022
@project-bot project-bot bot moved this from In Progress to Needs Review in Current Sprint (7/20 - 8/9) Aug 15, 2022
@josh-bagwell
Copy link
Contributor

Manny and I discussed alternative approaches. One option was to move the buttons outside the Modal.Body component and that fixes the issue. However, it doesn't address the issue of links/buttons losing focus ring in Modal.Body. It looks as if the overflow-y is causing the focus ring issue.

@mannycarrera4 mannycarrera4 added the blocked This issue is waiting for one or more issues to be closed/resolved. label Aug 23, 2022
@@ -43,7 +42,7 @@ export const BodyOverflow = () => {
<p>Are you sure you want to delete the item?</p>
<p>Are you sure you want to delete the item?</p>
</Modal.Body>
<HStack spacing="s" paddingTop="s">
<HStack spacing="s" paddingTop="s" paddingX="l" paddingBottom="l">
Copy link
Member

Choose a reason for hiding this comment

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

Now that we've removed Modal.Footer, it feels odd to me to require consumers to know they need paddingX="l" and paddingBottom="l" to get their HStack Footer to render with complementary padding to match Modal.Body and Modal.Heading. Previously, this was padding was baked into Modal.Footer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if we make this a Minor release, it makes sense to add this back in. But I agree the consumer would have to know what padding to add

Copy link
Member

Choose a reason for hiding this comment

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

For now, the consumer will need to add their own footer (using an HStack, for instance) with the appropriate paddings and margins to align with the Heading and Body subcomponents (see screenshot in top-level PR comment). We'll update Popup, Modal, and Dialog examples to include example footers with the needed padding and margin.

@mannycarrera4 mannycarrera4 changed the base branch from prerelease/minor to master September 8, 2022 18:18
@mannycarrera4 mannycarrera4 removed the on hold PR is on hold until further notice label Sep 8, 2022
@mannycarrera4 mannycarrera4 added approved Code has been reviewed and approved (ship it) and removed review in progress Code is currently under review labels Sep 9, 2022
@mannycarrera4 mannycarrera4 merged commit 0991b55 into Workday:master Sep 9, 2022
@jamesfan jamesfan mentioned this pull request Oct 4, 2022
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
7.x approved Code has been reviewed and approved (ship it) bug Something isn't working
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Buttons placed within Popup.Body are getting their focus rings cut off
5 participants