feat(range): updating the range pin to always be visible when enabled for ionic theme#29988
feat(range): updating the range pin to always be visible when enabled for ionic theme#29988OS-pedrolourenco merged 34 commits intonextfrom
Conversation
# Conflicts: # core/src/components/range/range.ionic.scss
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
brandyscarney
left a comment
brandyscarney
left a comment
There was a problem hiding this comment.
All of my requested changes are dependent on the --height variable being set to 40px as requested on #29979. This will make the total range height equal to 68px, matching Figma.
There was a problem hiding this comment.
Line 66 should be:
@include padding(calc(globals.$ion-space-100 + globals.$ion-scale-600), null, null, null);This will make it so the padding top is the height of the pin text + the space between the knob and pin text
# Conflicts: # core/src/components/range/test/basic/range.e2e.ts-snapshots/range-pin-ionic-md-ltr-light-Mobile-Chrome-linux.png # core/src/components/range/test/basic/range.e2e.ts-snapshots/range-pin-ionic-md-ltr-light-Mobile-Firefox-linux.png # core/src/components/range/test/basic/range.e2e.ts-snapshots/range-pin-ionic-md-ltr-light-Mobile-Safari-linux.png
tanner-reits
left a comment
tanner-reits
left a comment
There was a problem hiding this comment.
One suggestion to eliminate changes in the component
core/src/components/range/range.tsx
Outdated
| {pin && ( | ||
| <div class="range-pin" role="presentation" part="pin"> | ||
| {pinFormatter(value)} | ||
| {pinFormatter(value) + (theme === 'ionic' ? '%' : '')} |
There was a problem hiding this comment.
What do you think about changing the pinFormatter prop to return number | string. That way, we don't have to add this theme specific logic, and the OS widget could provide its own formatter that would append the %
There was a problem hiding this comment.
Sounds great! I will do that
# Conflicts: # core/src/components/range/range.ionic.scss # core/src/components/range/test/basic/range.e2e.ts-snapshots/range-pin-ionic-md-ltr-light-Mobile-Chrome-linux.png # core/src/components/range/test/basic/range.e2e.ts-snapshots/range-pin-ionic-md-ltr-light-Mobile-Firefox-linux.png # core/src/components/range/test/basic/range.e2e.ts-snapshots/range-pin-ionic-md-ltr-light-Mobile-Safari-linux.png
|
LGTM! |
3 participants
Issue number: internal
What is the current behavior?
The ion-range pin is only shown we the range is being dragged for all themes.
What is the new behavior?
Does this introduce a breaking change?
Other information