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: legacy radios in ion-select creates retainer when used in popover #27786

Closed
3 tasks done
sejalshah1107 opened this issue Jul 12, 2023 · 5 comments · Fixed by #27818
Closed
3 tasks done

bug: legacy radios in ion-select creates retainer when used in popover #27786

sejalshah1107 opened this issue Jul 12, 2023 · 5 comments · Fixed by #27818
Labels
package: core @ionic/core package type: bug a confirmed bug report

Comments

@sejalshah1107
Copy link

Prerequisites

Ionic Framework Version

v7.x

Current Behavior

I have created a 'ion-select' with interface set as popover.

On selection of an select-option, the DOM Nodes created for displaying the options are not released

Expected Behavior

DOM Nodes should be released upon selection / dismissal

Steps to Reproduce

    <ion-select interface="popover" placeholder="testing">
      <ion-select-option *ngFor="let a of [1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1], let i = index">
        {{ "page " + (i + 1) }}
      </ion-select-option>
    </ion-select>

the above following code generates the said behaviour. DOM Node count can be monitored in the 'Performance Monitor' tool

Code Reproduction URL

No response

Ionic Info

Ionic:

Ionic CLI : 7.1.1 (/usr/local/lib/node_modules/@ionic/cli)
Ionic Framework : @ionic/angular 7.0.2
@angular-devkit/build-angular : 15.2.6
@angular-devkit/schematics : 15.2.8
@angular/cli : 15.2.8
@ionic/angular-toolkit : 9.0.0

Capacitor:

Capacitor CLI : 4.7.3
@capacitor/android : not installed
@capacitor/core : 4.7.3
@capacitor/ios : not installed

Utility:

cordova-res : not installed globally
native-run : 1.7.2

System:

NodeJS : v18.16.0 (/home/yg/.nvm/versions/node/v18.16.0/bin/node)
npm : 9.5.1
OS : Linux 5.15

Additional Information

No response

@ionitron-bot ionitron-bot bot added the triage label Jul 12, 2023
@liamdebeasi liamdebeasi added the ionitron: needs reproduction a code reproduction is needed from the issue author label Jul 12, 2023
@ionitron-bot
Copy link

ionitron-bot bot commented Jul 12, 2023

Thanks for the issue! This issue has been labeled as needs reproduction. This label is added to issues that need a code reproduction.

Please reproduce this issue in an Ionic starter application and provide a way for us to access it (GitHub repo, StackBlitz, etc). Without a reliable code reproduction, it is unlikely we will be able to resolve the issue, leading to it being closed.

If you have already provided a code snippet and are seeing this message, it is likely that the code snippet was not enough for our team to reproduce the issue.

For a guide on how to create a good reproduction, see our Contributing Guide.

@ionitron-bot ionitron-bot bot removed the triage label Jul 12, 2023
@sejalshah1107
Copy link
Author

thank you for your response.

as required, please find the code checked in at https://github.com/sejalshah1107/ionic_bugs

best,
sejal

@amandaejohnston amandaejohnston added triage and removed ionitron: needs reproduction a code reproduction is needed from the issue author labels Jul 18, 2023
@liamdebeasi liamdebeasi self-assigned this Jul 18, 2023
@liamdebeasi
Copy link
Contributor

Thanks for the reproduction. I can reproduce the reported behavior, and it looks like this impacts the legacy form controls and not the modern form controls. We are planning to update the form controls in ion-select to use the modern syntax in #27071, so this issue should be resolved once #27071 is fixed.

@liamdebeasi liamdebeasi changed the title bug: ion-select with interface 'popover' is not releasing DOM Nodes after selection bug: legacy radios in ion-select creates retainer when used in popover Jul 18, 2023
@liamdebeasi liamdebeasi added package: core @ionic/core package type: bug a confirmed bug report labels Jul 18, 2023
@ionitron-bot ionitron-bot bot removed the triage label Jul 18, 2023
@liamdebeasi liamdebeasi removed their assignment Jul 18, 2023
github-merge-queue bot pushed a commit that referenced this issue Aug 1, 2023
Issue number: resolves #27071, resolves #27786

---------

<!-- 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?
<!-- Please describe the current behavior that you are modifying. -->

`ion-select-popover` is using the legacy syntax for `ion-checkbox` and
`ion-radio`.

## What is the new behavior?
<!-- Please describe the behavior or changes that are being added by
this PR. -->

- Migrated checkbox and radio to use the modern syntax
- Updated the visual rendering tests to also check the checked state. I
noticed during development that I accidentally broke the checked state,
so I added tests so we can't accidentally break it again.

A couple notes on other screenshot diffs:

- On iOS, the item border line now extends past the radio circle. This
is an intentional behavior and aligns with native iOS. Having the radio
in the "start" slot is typically only done when the content in the
default slot is another interactive element (like and input) and not a
text label.
- On MD, the control heights are smaller. When comparing with
https://material-components.github.io/material-components-web-catalog/#/component/select,
the new behavior seems to be correct. Both the spec and Ionic item
heights are 48px. Interestingly, the radios in `ion-select-popover` have
always been 48px tall, but the checkboxes were 54px. Now both the radios
and checkboxes are 48px.

## Does this introduce a breaking change?

- [ ] Yes
- [x] No

<!-- If this introduces a breaking change, please describe the impact
and migration path for existing applications below. -->


## 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: amandaejohnston <amandaejohnston@users.noreply.github.com>
Co-authored-by: ionitron <hi@ionicframework.com>
@liamdebeasi
Copy link
Contributor

Thanks for the issue. This has been resolved via #27818, and a fix will be available in an upcoming release of Ionic Framework.

@ionitron-bot
Copy link

ionitron-bot bot commented Aug 31, 2023

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 31, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
package: core @ionic/core package type: bug a confirmed bug report
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants