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

bug: Overlays override id assigned through htmlAttributes #29712

Closed
3 tasks done
sean-perkins opened this issue Jul 15, 2024 · 4 comments · Fixed by #29722 · 4 remaining pull requests
Closed
3 tasks done

bug: Overlays override id assigned through htmlAttributes #29712

sean-perkins opened this issue Jul 15, 2024 · 4 comments · Fixed by #29722 · 4 remaining pull requests
Labels
help wanted a good issue for the community package: core @ionic/core package type: bug a confirmed bug report

Comments

@sean-perkins
Copy link
Contributor

sean-perkins commented Jul 15, 2024

Prerequisites

Ionic Framework Version

v8.x

Current Behavior

Split from: #29704

In Ionic v8, overlays are overriding the id specified in the htmlAttributes with the incremental overlay ID assigned by Ionic Framework.

Applies to: modal, popover, action-sheet, loading, picker-legacy and toast.

Expected Behavior

Ionic's overlay should not assign the incremental overlay ID if the overlay already has an ID assigned to it by either the id attribute directly or the htmlAttributes.

Steps to Reproduce

  1. Create a new overlay, giving it a custom ID through htmlAttributes
  2. Observe that the overlay does not have the custom ID

This applies to all overlays except ion-alert, which has been resolved: #29708

For the reproduction URL we can either use the original reproduction and replace the alert with another overlay or assign htmlAttributes with an id property to any other overlay to observe the problem.

Code Reproduction URL

N/A

Ionic Info

N/A - reproduces in Ionic v8+.

Additional Information

No response

@sean-perkins sean-perkins added help wanted a good issue for the community package: core @ionic/core package type: bug a confirmed bug report labels Jul 15, 2024
Copy link

ionitron-bot bot commented Jul 15, 2024

This issue has been labeled as help wanted. This label is added to issues that we believe would be good for contributors.

If you'd like to work on this issue, please comment here letting us know that you would like to submit a pull request for it. This helps us to keep track of the pull request and make sure there isn't duplicated effort.

For a guide on how to create a pull request and test this project locally to see your changes, see our contributing documentation.

Thank you!

@sean-perkins
Copy link
Contributor Author

If anyone from the community is interesting in contributing, you can reference #29708 for the changes required to fix this issue (same approach would be applied to other overlays) and the test code to validate the fix.

@mikelhamer
Copy link
Contributor

I'm going to pick this up and start work!

github-merge-queue bot pushed a commit that referenced this issue Jul 24, 2024
Issue number: resolves #29712

---------

<!-- Please do not submit updates to dependencies unless it fixes an
issue. -->

<!-- Please try to limit your pull request to one type (bugfix, feature,
etc). Submit multiple pull requests if needed. -->

## 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
- [x] No

<!--
  If this introduces a breaking change:
1. Describe the impact and migration path for existing applications
below.
  2. Update the BREAKING.md file with the breaking change.
3. Add "BREAKING CHANGE: [...]" to the commit description when merging.
See
https://github.com/ionic-team/ionic-framework/blob/main/docs/CONTRIBUTING.md#footer
for more information.
-->


## Other information

<!-- Any other information that is important to this PR such as
screenshots of how the component looks before and after the change. -->

---------

Co-authored-by: Sean Perkins <13732623+sean-perkins@users.noreply.github.com>
Copy link

ionitron-bot bot commented Aug 23, 2024

Thanks for the issue! This issue is being locked to prevent comments that are not relevant to the original issue. If this is still an issue with the latest version of Ionic, please create a new issue and ensure the template is fully filled out.

@ionitron-bot ionitron-bot bot locked and limited conversation to collaborators Aug 23, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.