Skip to content
This repository has been archived by the owner on May 24, 2024. It is now read-only.

[terra-popup] Changed disabledHeader to true as default #2171

Closed
wants to merge 8 commits into from

Conversation

adavijit
Copy link
Collaborator

@adavijit adavijit commented May 21, 2024

Summary

What was changed:

  • Enabled close button for popup
  • Modified tests for the close button

Why it was changed:
Should be able to focus close button to close the popup

Testing

This change was tested using:

  • WDIO
  • Jest
  • Visual testing (please attach a screenshot or recording)
  • Other (please describe below)
  • No tests are needed

Reviews

In addition to engineering reviews, this PR needs:

  • UX review
  • Accessibility review
  • Functional review

Additional Details

This PR resolves:

UXPLATFORM-10398


Thank you for contributing to Terra.
@cerner/terra

saket2403
saket2403 previously approved these changes May 22, 2024
@saket2403 saket2403 changed the title [terra-popup] Changed disabledHeader to false as default [terra-popup] Changed disabledHeader to true as default May 22, 2024
@sugan2416
Copy link
Contributor

@adavijit

  • Doc examples of popup are causing page break
image
  • popup content area has reduced, adding a vertical scroll. Can you check why this has changed and since when?
image
  • Since all test examples of popup have set isHeaderDisabled to false, shouldn't doc examples be updated as well?

@saket2403 saket2403 self-requested a review May 22, 2024 10:09
@saket2403 saket2403 dismissed their stale review May 22, 2024 10:10

Need to re discuss the feature

@sdadn
Copy link
Contributor

sdadn commented May 22, 2024

The targetRef prop was being set as required in #2158.

This is also a breaking change that also needs to be reverted.

@github-actions github-actions bot temporarily deployed to preview-pr-2171 May 22, 2024 13:56 Destroyed
Copy link
Contributor

Choose a reason for hiding this comment

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

We should add a note to 6.84.0 that it has a breaking change that's corrected in 6.86.0

@saket2403
Copy link
Contributor

saket2403 commented May 23, 2024

The targetRef prop was being set as required in #2158.

This is also a breaking change that also needs to be reverted.

It was previously required prop I had made it optional in the previous release and reverted the change in 2158 as the change was not required and the prop being optional would have broken the popup if not used.
As per latest discussion in JRB (see JIRA 10398) Closing this PR.

@saket2403 saket2403 closed this May 23, 2024
@saket2403 saket2403 deleted the UXPLATFORM-10398_default_false branch May 23, 2024 05:48
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants