Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat(badge): add support for hint feature #30213
base: next
Are you sure you want to change the base?
feat(badge): add support for hint feature #30213
Changes from 8 commits
6b145ce
eb4580e
45637ec
b17f267
768520f
e9415f2
bb7140a
0eb919f
a6c2643
a89dc35
2a0706a
8840982
6b90698
34e41b3
a010083
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
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:
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 byion-fab
and offerhorizontal
andvertical
options instead? This is how MUI handles it: https://mui.com/material-ui/react-badge/#badge-alignmentAlso, we should use
end
instead ofright
if it flips in RTL.There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 includetop-left
andbottom-left
?There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 withhorizontal
andvertical
. It would stay consistent with what we have, leading for an easier introduction for the community.There was a problem hiding this comment.
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 🤔
There was a problem hiding this comment.
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 acceptstop | bottom
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes for me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made the change 👍