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.

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

feat(badge): add support for hint feature #30213

Open
wants to merge 14 commits into
base: next
Choose a base branch
from
Open

feat(badge): add support for hint feature #30213

wants to merge 14 commits into from
+142 −7

Conversation

BenOsodrac
Copy link
Contributor

@BenOsodrac BenOsodrac commented Feb 25, 2025 •
edited
Loading

Issue number: internal

What is the new behavior?

  • Added new position prop.
  • Based on the prop, a new badge-position css class is added on the host.
  • Removed css rule that prevented the badge to be rendered when empty.
  • Added common and themes styles to support the position prop and new scale size (on ios/md defaults to min-width variable, as size is not yet supported on native themes).
  • Added new tests specific for this new feature and added new screenshots.
  • Support to properly work inside Avatar, Button and TabButton will be added on different tasks (in the meantime the snapshots will appear wrong for these use-cases)

Does this introduce a breaking change?

  • Yes
  • No

Other information

@BenOsodrac BenOsodrac added type: feature request a new feature, enhancement, or improvement package: core @ionic/core package labels Feb 25, 2025
@vercel Vercel
Copy link

vercel bot commented Feb 25, 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 Feb 27, 2025 9:37am

@github-actions github-actions bot added package: angular @ionic/angular package package: vue @ionic/vue package labels Feb 25, 2025
OS-martacarlos and others added 2 commits February 25, 2025 17:26
@OS-martacarlos @brandyscarney @BenOsodrac
feat(badge): add icon sizes for ionic theme (#30203)
e9415f2 
Issue number: internal

---------

<!-- 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. -->

Icons inside badges don't have any specific styling.

<!-- Please describe the behavior or changes that are being added by
this PR. -->
According to the design, for the ionic theme, the size of icons inside
badges have specific values. To accommodate those:
- Added new rules for the badge sizes
- Adding a new testing page and screenshot tests

- [ ] 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.
-->

<!-- 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: Brandy Smith <[email protected]>
@BenOsodrac
make screenshot smaller
bb7140a
@BenOsodrac BenOsodrac marked this pull request as ready for review February 25, 2025 17:55
@BenOsodrac BenOsodrac requested a review from a team as a code owner February 25, 2025 17:55
@BenOsodrac BenOsodrac requested review from OS-pedrolourenco and brandyscarney and removed request for OS-pedrolourenco February 25, 2025 17:55
brandyscarney
brandyscarney requested changes Feb 25, 2025
View reviewed changes
core/src/components/badge/badge.tsx Outdated
*
* Defaults to `"static"`.
*/
@Prop() position: 'top-right' | 'bottom-right' | 'static' = 'static';
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure about the property name and values. Here are some of the other components for reference:

Component Property Options
ion-infinite-scroll position "bottom" | "top"
ion-label position "fixed" | "floating" | "stacked"
ion-toast position "bottom" | "middle" | "top"
ion-fab horizontal "center" | "end" | "start"
ion-fab vertical "bottom" | "center" | "top"

Do we plan to eventually support a left property? Maybe we should follow the approach used by ion-fab and offer horizontal and vertical options instead? This is how MUI handles it: https://mui.com/material-ui/react-badge/#badge-alignment

Also, we should use end instead of right if it flips in RTL.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not planned, but I thought this would scale better if that changes in the future. But I agree with you with the start/end stuff, will change that. Maybe will make it bottom | top, to make it similar to other components.

I think for this horizontal and vertical would be too much just for two positions.

Copy link
Member

Choose a reason for hiding this comment

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

What would static stand for? Should this be described as well? Any reason we don't include top-left and bottom-left?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My original idea for static, was to stand for the actual css property used, position: static, as in no absolute position is being used.

But after @brandyscarney feedback I changed to just top and bottom. As for other positions, according to UX team only the right/end side makes sense for them.

Copy link
Contributor

Choose a reason for hiding this comment

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

My vote is to follow what fab does with horizontal and vertical. It would stay consistent with what we have, leading for an easier introduction for the community.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But there's also position in ion-infinite-scroll, ion-label and ion-toast. Personally, when I see horizontal or vertical props, I think about alignment, not absolute positioning 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@brandyscarney @thetaPC @christian-bromann do you all vote for the ion-fab approach and instead have a vertical prop that accepts top | bottom ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes for me

Copy link
Member

Choose a reason for hiding this comment

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

+1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made the change 👍

@BenOsodrac BenOsodrac requested a review from thetaPC February 27, 2025 10:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@christian-bromann christian-bromann christian-bromann left review comments

@brandyscarney brandyscarney Awaiting requested review from brandyscarney

@thetaPC thetaPC Awaiting requested review from thetaPC

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
package: angular @ionic/angular package package: core @ionic/core package package: vue @ionic/vue package type: feature request a new feature, enhancement, or improvement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@BenOsodrac @christian-bromann @brandyscarney @thetaPC @Ionitron @OS-martacarlos