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

fix(select): ensure select sizes are respected when the label is empty #30087

Merged
merged 7 commits into from
Dec 20, 2024

Conversation

JoaoFerreira-FrontEnd
Copy link

@JoaoFerreira-FrontEnd JoaoFerreira-FrontEnd commented Dec 16, 2024

Issue number: internal


What is the current behavior?

Currently, the experience planned for ionic select has the sizes fixed and should be respected even when the labels are not presented.
https://stackblitz.com/edit/ku3fqz?file=src%2Fmain.tsx

What is the new behavior?

Does this introduce a breaking change?

  • Yes
  • No

@JoaoFerreira-FrontEnd JoaoFerreira-FrontEnd added package: core @ionic/core package type: bug a confirmed bug report labels Dec 16, 2024
@JoaoFerreira-FrontEnd JoaoFerreira-FrontEnd requested a review from a team as a code owner December 16, 2024 11:12
Copy link

vercel bot commented Dec 16, 2024

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 Dec 20, 2024 4:29pm

@JoaoFerreira-FrontEnd JoaoFerreira-FrontEnd changed the title fix(select): Select sizes are not respected when the label is not filled. fix(select): select sizes are not respected when the label is not filled. Dec 16, 2024
@thetaPC
Copy link
Contributor

thetaPC commented Dec 16, 2024

@JoaoFerreira-FrontEnd

Make sure to have the checks passing before requesting a review.

Please review how we write the PR descriptions. This one is missing information on the current behavior. It helps the reviewer understand the issue if there's text vs having to figure out the issue through a repro. It would have taken me some time to understand the issue if I hadn't already known about it prior. But it might have taken some time for someone else since there was no mention of sizes being affected by the lack of a label.

Please review how we write the PR titles. We don't end the titles with a period and there's no need to repeat the component name within the title since it's already mentioned in the scope. I would recommend something like: fix(select): ensure select sizes are respected when the label is empty

@JoaoFerreira-FrontEnd JoaoFerreira-FrontEnd changed the title fix(select): select sizes are not respected when the label is not filled. fix(select): ensure select sizes are respected when the label is empty Dec 17, 2024
- organize select styles so they don't affect ionic theme;
Copy link
Contributor

@thetaPC thetaPC left a comment

Choose a reason for hiding this comment

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

LGTM

- update all select snapshots;
- add empty label select to test page;
@JoaoFerreira-FrontEnd JoaoFerreira-FrontEnd merged commit 3f00e69 into next Dec 20, 2024
46 checks passed
@JoaoFerreira-FrontEnd JoaoFerreira-FrontEnd deleted the ROU-11438 branch December 20, 2024 17:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: core @ionic/core package type: bug a confirmed bug report
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants