Skip to content

fix(overlays): do not overwrite id set in htmlAttributes#29722

Merged
sean-perkins merged 11 commits intoionic-team:mainfrom
mikelhamer:main
Jul 24, 2024
Merged

fix(overlays): do not overwrite id set in htmlAttributes#29722
sean-perkins merged 11 commits intoionic-team:mainfrom
mikelhamer:main

Conversation

@mikelhamer
Copy link
Copy Markdown
Contributor

@mikelhamer mikelhamer commented Jul 19, 2024 •
edited by sean-perkins
Loading

Issue number: resolves #29712

What is the current behavior?

In every type of overlay, the auto incremented overlay id is overwriting any id set in htmlAttributes.

What is the new behavior?

The id in htmlAttributes now takes precedence.

Does this introduce a breaking change?

  • Yes
  • No

Other information

@mikelhamer mikelhamer requested a review from a team as a code owner July 19, 2024 06:16
@mikelhamer mikelhamer requested a review from thetaPC July 19, 2024 06:16
@vercel
Copy link
Copy Markdown

vercel Bot commented Jul 19, 2024 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
ionic-framework ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 19, 2024 2:38pm

@github-actions github-actions Bot added the package: core @ionic/core package label Jul 19, 2024
@mikelhamer
Copy link
Copy Markdown
Contributor Author

mikelhamer commented Jul 19, 2024

@sean-perkins I've created this PR to fix #29712 , but something I noticed is that the overlay ID is being used in the picker-legacy component, but not in the the new picker component.
Should overlay ID be added there as well, along with making sure it doesn't overwrite the id in htmlAttributes?

@sean-perkins
Copy link
Copy Markdown
Contributor

sean-perkins commented Jul 19, 2024

@mikelhamer great observation, you are correct in your current implementation pattern. ion-picker-legacy requires this fix, but ion-picker is not an overlay, it is optionally presented in one if a developer configures it with a popover or modal. I've updated the original issue description to correct this.

@mikelhamer
Copy link
Copy Markdown
Contributor Author

mikelhamer commented Jul 19, 2024

Awesome! The more you know... 🌈 ⭐️

sean-perkins
sean-perkins suggested changes Jul 19, 2024
View reviewed changes
Comment thread core/src/components/popover/popover.tsx Outdated
@mikelhamer @sean-perkins
fix(popover): use nullish coalescing to ensure htmlAttributes id is n…
cb02c3e 
…ot overwritten when it has an integer value of 0

Co-authored-by: Sean Perkins <13732623+sean-perkins@users.noreply.github.com>
@mikelhamer mikelhamer requested a review from sean-perkins July 21, 2024 17:10
sean-perkins
sean-perkins approved these changes Jul 22, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@sean-perkins sean-perkins left a comment

Choose a reason for hiding this comment

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

Awesome 👏 thanks for knocking these out!

@sean-perkins sean-perkins enabled auto-merge July 22, 2024 17:54
@mikelhamer
Copy link
Copy Markdown
Contributor Author

mikelhamer commented Jul 22, 2024

My pleasure! It was an awesome learning experience.

brandyscarney
brandyscarney approved these changes Jul 24, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@brandyscarney brandyscarney left a comment

Choose a reason for hiding this comment

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

Thank you for the PR! Great work. 🎉

@sean-perkins sean-perkins added this pull request to the merge queue Jul 24, 2024
Merged via the queue into ionic-team:main with commit 92ce563 Jul 24, 2024
@mikelhamer
Copy link
Copy Markdown
Contributor Author

mikelhamer commented Jul 24, 2024

Thank you! 🚀

@vinnivitz vinnivitz mentioned this pull request Aug 15, 2024
Open
This was referenced Aug 25, 2024
Open
Open
@vinnivitz vinnivitz mentioned this pull request Sep 4, 2024
Merged
This was referenced Sep 21, 2024
Open
Open
@bdr0id bdr0id mentioned this pull request Oct 26, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@brandyscarney brandyscarney brandyscarney approved these changes

@thetaPC thetaPC Awaiting requested review from thetaPC thetaPC is a code owner automatically assigned from ionic-team/framework

+1 more reviewer

@sean-perkins sean-perkins sean-perkins approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

package: core @ionic/core package

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

bug: Overlays override id assigned through htmlAttributes

3 participants

@mikelhamer @sean-perkins @brandyscarney