vmware-clarity / core

Clarity is a scalable, accessible, customizable, open-source design system built with web components. Works with any JavaScript framework, created for enterprises, and designed to be inclusive.
https://clarity.design
MIT License
166 stars 42 forks source link

fix(modal): react - fix modal visibility when hidden="false" #123

Closed ashleyryan closed 2 years ago

ashleyryan commented 2 years ago

fixes #122

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

What is the current behavior?

hidden={false} on CdsModal renders as hidden="false", which isn't really valid html, but it's an issue with react. The modal then doesn't render, because of css selectors for [hidden]

We had CSS to handle this case in v5, but some of the overlay/popover work in v6 overwrote the CSS

Issue Number: #122

What is the new behavior?

Increase CSS specificity of the visible styling for hidden="false"

Does this PR introduce a breaking change?

Other information

This could have been fixed alternatively with a React HOC that converts the hidden boolean value of false to undefined, which prevents the hidden attribute from being applied as expected. But in the interest of keeping the React code base as thin as possible, I put the fix into CSS that already existed for this use case.

I reported this to the Lit team on their slack in the hopes they'd come up with a fix in lit-labs/react, and this is something they're going to look into fixing more broadly. Reported here: https://github.com/lit/lit/issues/3053

Also, I tried to add a react unit tests for this, but there seemed to be a bug somewhere in testing-library/dom or js-dom for this where the visibility property in CSS isn't cascaded/inherited correctly and checking for visibility returned visible, even when it's not.

github-actions[bot] commented 2 years ago

πŸ‘‹ @ashleyryan,

Thank you,

πŸ€– Clarity Release Bot

github-actions[bot] commented 2 years ago

:tada: This PR is included in version 6.1.0 :tada:

The release is available on:

Your semantic-release bot :package::rocket:

github-actions[bot] commented 2 years ago

Hi there πŸ‘‹, this is an automated message. To help Clarity keep track of discussions, we automatically lock closed PRs after 14 days. Please look for another open issue or open a new issue with updated details and reference this one as necessary.