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

[ch-dialog][ch-popover] Rename property name hidden to show #469

Merged
merged 32 commits into from
Jan 7, 2025

Conversation

bsastregx
Copy link
Collaborator

@bsastregx bsastregx commented Jan 2, 2025

Changes in this PR:

Update the renamed hidden property in favor to show, on the components that use ch-popover:

  • ch-combo-box-render
  • ch-dropdown
  • ch-tooltip

Breaking changes

  • Rename property name hidden to show in the following components:
  • ch-popover
  • ch-dialog

Why this change?

The presence of the hidden property was causing issues on React, since "hidden" was being evaluated to true, regardless of the property value.

Copy link

netlify bot commented Jan 2, 2025

Deploy Preview for gx-chameleon ready!

Name Link
🔨 Latest commit c08aef7
🔍 Latest deploy log https://app.netlify.com/sites/gx-chameleon/deploys/677d5155fa606a0008aec2ff
😎 Deploy Preview https://deploy-preview-469--gx-chameleon.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.

@bsastregx bsastregx requested a review from ncamera January 2, 2025 18:53
src/components/combo-box/combo-box.tsx Outdated Show resolved Hide resolved
src/components/dialog/dialog.tsx Outdated Show resolved Hide resolved
src/components/dialog/dialog.tsx Outdated Show resolved Hide resolved
src/components/dialog/dialog.tsx Outdated Show resolved Hide resolved
src/components/dialog/dialog.tsx Outdated Show resolved Hide resolved
src/components/popover/popover.tsx Outdated Show resolved Hide resolved
src/components/popover/popover.tsx Outdated Show resolved Hide resolved
src/components/tooltip/tooltip.tsx Outdated Show resolved Hide resolved
src/components/dialog/dialog.tsx Outdated Show resolved Hide resolved
src/components/popover/popover.tsx Outdated Show resolved Hide resolved
@ncamera ncamera changed the title Rename property name hidden to show in several components [ch-dialog][ch-popover] Rename property name hidden to show Jan 2, 2025
@ncamera ncamera added fix Bug fix pull request target: minor This PR is targeted for the next minor release breaking changes labels Jan 2, 2025
@bsastregx bsastregx requested a review from ncamera January 3, 2025 19:59
src/components/dialog/dialog.tsx Outdated Show resolved Hide resolved
src/components/dialog/tests/constraints.e2e.ts Outdated Show resolved Hide resolved
src/components/dialog/tests/constraints.e2e.ts Outdated Show resolved Hide resolved
src/components/dialog/tests/constraints.e2e.ts Outdated Show resolved Hide resolved
src/components/dialog/tests/constraints.e2e.ts Outdated Show resolved Hide resolved
src/components/dialog/tests/constraints.e2e.ts Outdated Show resolved Hide resolved
src/components/dialog/tests/constraints.e2e.ts Outdated Show resolved Hide resolved
src/components/dialog/tests/constraints.e2e.ts Outdated Show resolved Hide resolved
src/components/dialog/tests/constraints.e2e.ts Outdated Show resolved Hide resolved
For the button to be rendered, `showHeader` property should be `true`
@bsastregx bsastregx requested a review from ncamera January 4, 2025 00:05
Copy link
Collaborator

@ncamera ncamera left a comment

Choose a reason for hiding this comment

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

We're almost there!

src/components/dialog/tests/accessibility.e2e.ts Outdated Show resolved Hide resolved
src/components/dialog/tests/basic.e2e.ts Outdated Show resolved Hide resolved
src/components/dialog/tests/basic.e2e.ts Outdated Show resolved Hide resolved
src/components/dialog/tests/basic.e2e.ts Outdated Show resolved Hide resolved
src/components/popover/popover.tsx Outdated Show resolved Hide resolved
src/components/dialog/dialog.tsx Outdated Show resolved Hide resolved
src/components/dialog/tests/accessibility.e2e.ts Outdated Show resolved Hide resolved
src/components/dialog/tests/basic.e2e.ts Outdated Show resolved Hide resolved
Some tests are failing on `basic.e2e.ts` only on the server. Check what happens if `testDefaultProperty` is not run.
Although they are not required at the time of writting, setting this property to true ensures future compatibility, in the case the parts involved on these tests are dependent on the `show` property to be rendered.
@bsastregx bsastregx requested a review from ncamera January 7, 2025 15:32
src/components/dialog/tests/basic.e2e.ts Outdated Show resolved Hide resolved
src/components/dialog/tests/basic.e2e.ts Outdated Show resolved Hide resolved
These types of checks should be asserted on another test suite rather.
@bsastregx bsastregx requested a review from ncamera January 7, 2025 16:17
Copy link
Collaborator

@ncamera ncamera left a comment

Choose a reason for hiding this comment

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

👍

@bsastregx bsastregx merged commit a016a74 into main Jan 7, 2025
7 checks passed
@bsastregx bsastregx deleted the refactor/rename-hidden-to-show branch January 7, 2025 16:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking changes fix Bug fix pull request target: minor This PR is targeted for the next minor release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants