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(modal): update hasScrollingContent styles #14712

Merged

Conversation

alisonjoseph
Copy link
Member

@alisonjoseph alisonjoseph commented Sep 25, 2023

Closes #13714

Update the styling for the scroll fade so that it appears inside the modal-content and there isn't an odd space below. This also fixes the issue where if you click in the space below the content it closes the modal.

Changelog

New

  • Add WithScrollingContent stories to Modal and ComposedModal

Changed

  • swap margin for padding on modal-content so the fade will appear inside the modal
  • update styles for modal-scroll-content to accommodate the margin/padding swap

Testing / Reviewing

Check styles for Modal and ComposedModal, focus should wrap entire content area above button.

@netlify
Copy link

netlify bot commented Sep 25, 2023

Deploy Preview for v11-carbon-react ready!

Name Link
🔨 Latest commit c429e18
🔍 Latest deploy log https://app.netlify.com/sites/v11-carbon-react/deploys/65145f55f815450008081255
😎 Deploy Preview https://deploy-preview-14712--v11-carbon-react.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@netlify
Copy link

netlify bot commented Sep 25, 2023

Deploy Preview for carbon-elements ready!

Name Link
🔨 Latest commit c429e18
🔍 Latest deploy log https://app.netlify.com/sites/carbon-elements/deploys/65145f550ce0240008ca5df2
😎 Deploy Preview https://deploy-preview-14712--carbon-elements.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@alisonjoseph alisonjoseph marked this pull request as ready for review September 26, 2023 17:39
@alisonjoseph alisonjoseph requested a review from a team as a code owner September 26, 2023 17:39
Copy link
Member

@tay1orjones tay1orjones left a comment

Choose a reason for hiding this comment

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

Looks good! Any idea what's up with the chevron alignment in the select? It looks fine in the select story and I can't pinpoint what style has it so misaligned 🤔

image

@alisonjoseph
Copy link
Member Author

alisonjoseph commented Sep 27, 2023

@tay1orjones ohh its the inline margin-bottom style I had added to the select, its adding it to the element and not the wrapping div. Not sure we can fix that in the component without it being a breaking change. I updated the story to add a wrapper div.

Copy link
Contributor

@andreancardona andreancardona left a comment

Choose a reason for hiding this comment

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

One last thing - top modal chevron looks a little misaligned with the others

Screenshot 2023-09-27 at 16 32 45

@alisonjoseph
Copy link
Member Author

alisonjoseph commented Sep 28, 2023

@andreancardona I already have a PR open to fix the chevron position (its unrelated to this PR) #14725

@github-actions github-actions bot added this pull request to the merge queue Sep 28, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Sep 28, 2023
@alisonjoseph alisonjoseph merged commit 435e72d into carbon-design-system:main Oct 5, 2023
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[a11y]: Modal: The 'fade' effect on dialogs with overflow body text creates some odd behaviours
3 participants