Skip to content

fix(select): update icon color and use correct focused class #30342

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 18 commits into from
Apr 16, 2025
Merged

fix(select): update icon color and use correct focused class #30342

merged 18 commits into from
Apr 16, 2025

Conversation

thetaPC
Copy link
Contributor

@thetaPC thetaPC commented Apr 7, 2025 •
edited
Loading

Issue number: internal

What is the current behavior?

There are a few items to note:

  • Styles are using a non-existing focus class. It currently uses .has-focus which isn't being used anywhere within select.tsx. It seems that .has-focus comes a copy and paste that wasn't updated.
  • Icon uses the highlight color when expanded, in item, and no fill. This leads to the styling looking weird compared to when it's not in an item especially since labels do not change colors. Only applies to md mode.
List no lines List with lines
Screenshot 2025-04-07 at 1 01 14 PM Screenshot 2025-04-07 at 1 01 31 PM
  • The focus without a validation status does not update the border correctly when inside an item and has a solid fill. Only applies to md mode.
Outside item Inside item
Screenshot 2025-04-07 at 1 07 13 PM Screenshot 2025-04-07 at 1 08 45 PM

What is the new behavior?

  • Styles are now using the correct focus class: .has-focus, removing ion-focused.
  • If the select is inside an item and has no fill then the icon will not use the highlight color. Only applies to md mode.
List no lines List with lines
Screenshot 2025-04-07 at 1 13 17 PM Screenshot 2025-04-07 at 1 14 11 PM
  • The focus without a validation status uses the highlight color when inside an item and has a solid fill. Only applies to md mode.
Outside item Inside item
Screenshot 2025-04-07 at 1 07 13 PM Screenshot 2025-04-07 at 1 16 47 PM

Does this introduce a breaking change?

  • Yes
  • No

Other information

Dev build: 8.5.4-dev.11744743162.1ec9251d

Select cannot have a focused class and an expanded class at the same time. It's one or the other.

@vercel Vercel
Copy link

vercel bot commented Apr 7, 2025 •
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 Apr 15, 2025 8:04pm

@github-actions github-actions bot added the package: core @ionic/core package label Apr 7, 2025
thetaPC
thetaPC commented Apr 7, 2025
View reviewed changes
core/src/components/select/select.md.scss
Comment on lines -15 to -22
.select-icon {
width: $select-md-icon-size;

transition: transform .15s cubic-bezier(.4, 0, .2, 1);

color: #{$text-color-step-500};
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just moved this down to the select icon section.

@thetaPC thetaPC changed the title refactor(select): use gray on icon when expanded, in item, and no fill refactor(select): update highlight color and use correct focused class Apr 7, 2025
@thetaPC thetaPC marked this pull request as ready for review April 7, 2025 20:28
@thetaPC thetaPC requested a review from a team as a code owner April 7, 2025 20:28
@thetaPC thetaPC requested review from brandyscarney and ShaneK April 7, 2025 20:28
@thetaPC thetaPC changed the title refactor(select): update highlight color and use correct focused class fix(select): update highlight color and use correct focused class Apr 8, 2025
@thetaPC thetaPC changed the title fix(select): update highlight color and use correct focused class fix(select): update icon color and use correct focused class Apr 11, 2025
brandyscarney
brandyscarney reviewed Apr 14, 2025
View reviewed changes
Copy link
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.

A couple comments regarding the valid/invalid behavior since we removed the focus color from the icon.

brandyscarney
brandyscarney approved these changes Apr 15, 2025
View reviewed changes
Copy link
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.

Looks good! Just added some minor corrections for some comments.

thetaPC and others added 2 commits April 15, 2025 13:01
@thetaPC @brandyscarney
Update core/src/components/select/select.md.solid.scss
33ca720 
Co-authored-by: Brandy Smith <brandyscarney@users.noreply.github.com>
@thetaPC @brandyscarney
Update core/src/components/select/select.md.outline.scss
db25824 
Co-authored-by: Brandy Smith <brandyscarney@users.noreply.github.com>
ShaneK
ShaneK approved these changes Apr 16, 2025
View reviewed changes
Copy link
Member

@ShaneK ShaneK left a comment

Choose a reason for hiding this comment

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

Looks good to me, great job!

@thetaPC thetaPC added this pull request to the merge queue Apr 16, 2025
Merged via the queue into main with commit cad1c61 Apr 16, 2025
50 checks passed
@thetaPC thetaPC deleted the FW-6265 branch April 16, 2025 17:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ShaneK ShaneK ShaneK approved these changes

@brandyscarney brandyscarney brandyscarney approved these changes

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.
3 participants
@thetaPC @ShaneK @brandyscarney