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

USWDS - Modal: Remove padding style rules after modal close #5493

Merged
merged 8 commits into from Sep 28, 2023

Conversation

amyleadem
Copy link
Contributor

@amyleadem amyleadem commented Sep 8, 2023

Summary

Updated how the modal JavaScript resets body padding. Now the component will rely on the CSS to reset the body padding rather than injecting an inline style with JavaScript.

Improved body padding detection in modal JavaScript. The JS now checks for body padding after the modal has been set up.

Breaking change

This is not a breaking change.

Related issue

Closes #5361

Related pull requests

Changelog PR

Preview link

Problem statement

When the modal opens, it adds a temporary padding to the body to account for the missing scrollbar. When the modal closes, it is expected that the padding resets to the body's initial padding definition. For example, the Storybook body has an initial padding value of 1rem, and the expectation is that the body styles would return to 1rem when the modal is closed. However, in the current state in Storybook, the modal JS changes the body padding to 0px when the modal is closed.

This appears to only happen in Storybook and could not be replicated in uswds-sandbox or uswds-site. I believe it is related to how Storybook initializes.

Solution

  1. The JavaScript now reverts back to the original CSS styling when the modal is closed, rather than declaring another inline style. Here is a description of the before and after (note the change in the last step of the process):
    • In develop, the component changes the body padding using the following methods:
      • The initial body padding is set with CSS (For example, 12px)
      • Once the modal is activated, the JS adds an inline style rule to the body element that updates the padding to include the width of the scrollbar (For example, 27px).
      • Then when the modal is closed the JS resets the definition of the inline style rule to the the initial value of 12px.
    • In this PR, the component changes the body padding using the following methods:
      • The initial body padding is set with CSS (For example, 12px)
      • Once the modal is activated, the JS adds an inline style rule to the body element that updates the padding to include the width of the scrollbar (For example, 27px).
      • Then when the modal is closed the JS removes the padding style rule that it added and the padding is defined with the original CSS again.
  2. The JavaScript now explicitly checks for body padding style during the modal is set up. This allowed the JS to recognize the body padding in Storybook.

Testing and review

  • Confirm that the body padding behaves as expected when the modal is opened/closed.
  • Confirm no regressions in modal behavior.
  • Consider if there is any risk to this change.

In Storybook:

  1. Resize the window to 450px
  2. Check the initial padding on the body element (Should be 1em, defined in the CSS)
  3. Open the modal
  4. Check the updated padding on the body element (Should be 32px, defined in an inline style rule)
    • Note: The 32px value is created by adding 17px (the computed padding on the body) and 15px (the computed width of the scrollbar)
  5. Close the modal
  6. Confirm that the inline body padding style was removed and that the body padding is restored to the original value, defined by the CSS (1em)

In the sandbox demo or USWDS-site demo:

  1. Check the initial padding on the body element (Should be 38px, defined in the CSS)
  2. Open the modal
  3. Check the updated padding on the body element (Should be 53px, defined in an inline style rule)
  4. Close the modal
  5. Confirm that the body padding is restored to the original value (Should be 38px, defined in the CSS)

@amyleadem amyleadem marked this pull request as ready for review September 12, 2023 23:06
@mahoneycm
Copy link
Contributor

Flagging that we might have to update #5315 to include changes made by this PR

Copy link
Contributor

@mejiaj mejiaj left a comment

Choose a reason for hiding this comment

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

I was able to confirm the original issue is specific to StorybookJS and its known init issues.

How I confirmed

  • Using StorybookJS: Confirming padding was reported as 0 initially. We call start.js in .storybook/preview.js to initialize components. In this file we're checking for two DOM states; one for DOMReady and another for DOM already loaded [best practice outlined in Document: DOMContentLoaded event - Web APIs | MDN].
  • USWDS Sandbox: Added padding and got the correct values on toggle.

It seems like a better experience to move the padding check closer to the modal toggle. Is it possible to handle padding when toggleModal() is called?

packages/usa-modal/src/index.js Show resolved Hide resolved
@@ -49,7 +42,7 @@ const onMenuClose = () => {
* @param {KeyboardEvent} event the keydown event
* @returns {boolean} safeActive if mobile is open
*/
function toggleModal(event) {
function toggleModal(event, temporaryBodyPadding) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I like the idea of improving the accuracy of the padding by checking on init, but we should check for padding inside of the toggle or at the top-level.

@amyleadem
Copy link
Contributor Author

amyleadem commented Sep 20, 2023

@mejiaj I moved the padding check to setUpModal() because the goal is just to capture and reuse the initial value. Curious what you think!

@amyleadem
Copy link
Contributor Author

@mahoneycm Can you let me know if the component still behaves as expected, even if the actual padding values vary?

Copy link
Contributor

@mahoneycm mahoneycm left a comment

Choose a reason for hiding this comment

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

Values are differing for me but it IS working as expected!

  • Initial padding matches up
  • Opening the modal does not cause content shift
  • Opening modal updates body's padding via inline styles
  • Closing the modal reverts padding to initial state!

Copy link
Contributor

@mejiaj mejiaj left a comment

Choose a reason for hiding this comment

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

Small question on the removal of all styles on body, otherwise LGTM.

packages/usa-modal/src/index.js Outdated Show resolved Hide resolved
Copy link
Contributor

@mejiaj mejiaj left a comment

Choose a reason for hiding this comment

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

Nice fix, thanks!

@thisisdano thisisdano merged commit 1aeb43a into develop Sep 28, 2023
5 checks passed
@thisisdano thisisdano mentioned this pull request Sep 29, 2023
@mahoneycm mahoneycm mentioned this pull request Oct 16, 2023
7 tasks
werdnanoslen added a commit to trussworks/react-uswds that referenced this pull request Oct 30, 2023
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.

USWDS - Bug: Modal removes existing padding on close
4 participants