feat(input): add outline appearance for stacked label to ionic theme #29268
Conversation
amandaejohnston
changed the title
| :host { | ||
| --border-width: 1px; | ||
| --border-color: #B8B8B8; | ||
| } |
There was a problem hiding this comment.
I went ahead and put these styles here instead of the outline-specific file since they'll be needed for the underline appearance as well.
There was a problem hiding this comment.
This change is due to the default label placement changing to stacked for this theme.
There was a problem hiding this comment.
While this looks broken currently, I intentionally left it as-is because the test uses a floating label placement, which this ticket explicitly doesn't cover. When floating labels are implemented later, these screenshots will be fixed. LMK if you'd like me to change the relevant size="large" tests to use stacked instead.
amandaejohnston
requested review from
sean-perkins
and removed request for
a team
There was a problem hiding this comment.
I think the implementation should adjust the rendering of the outline container to not include MD specific elements for the ionic theme.
This would simplify the styling and the cognitive load of trying to understand why we are rendering an element and then visually hiding it for a theme.
| const theme = getIonTheme(this); | ||
| const { el, labelPlacement } = this; | ||
|
|
||
| if (theme === 'ionic' && labelPlacement !== 'stacked' && labelPlacement !== 'floating') { |
There was a problem hiding this comment.
~ I don't know if we need the overhead for runtime checks for every unsupported prop/prop option for a given theme.
Open to team feedback on this, but this feels like a problem to solve closer to beta/rc/ga. We may fill these feature gaps in that time and the documentation may be clear enough for developers to navigate.
There was a problem hiding this comment.
I'm open to removing the check and/or warning, but it was included in the design doc and is a pattern we're already using for other features. The size prop on ion-input is already implemented this way, for example.
| position: absolute; | ||
|
|
||
| /** | ||
| * Label text should not extend |
There was a problem hiding this comment.
~ this looks to be copy and paste, we could probably remove the 40 character line wrapping since the file already extends wider than this line would be.
There was a problem hiding this comment.
True that this was copy-pasted, but it's just Liam's commenting style; this sort of thing is all over our codebase right now 😄
There was a problem hiding this comment.
- The label doesn't have the correct font color (
#373737). - The placeholder color is not correct. (
#535353). - The font size for input is 14px instead of 16px. Also can't tell if label needs adjusting too.
There was a problem hiding this comment.
Colors updated in 1f0a574. Font size seems like more of a global typography thing? The default font size for the ionic theme is 14px, and ion-input's font size is currently set to inherit, so that should just work once everything is put together. At least, as far as I understand it.
There was a problem hiding this comment.
Hmm, I wonder who we should check in to verify this?
There was a problem hiding this comment.
I checked with Sean in Slack. The font used in the screenshots is hardcoded in a separate file and affects all ionic theme screenshots, so changing it should be out of scope for this PR. I made a ticket to track this: https://outsystemsrd.atlassian.net/browse/FW-6116
Added some styles in d17c865, screenshots incoming once I fix up everything else. It looks like the default ion-icon size is already 16px as prescribed, so I only touched the margins; I figure any further sizing details should be up to the app since slots are designed for more flexibility. |
|
There are some build differences from |
Co-authored-by: Maria Hutt <thetaPC@users.noreply.github.com>
Issue number: Internal
What is the new behavior?
All changes are specific to the
ionictheme.fill="outline"pluslabelPlacement="stacked".labelPlacementis now"stacked".labelPlacementbesides"stacked"and"floating"cannot be used.Note that per the ticket, I did not account for any other scope, including styles for helper text,
labelPlacement="floating",shape="round", etc. This means that some states will look broken for now, and will be addressed in future tickets.Does this introduce a breaking change?
Other information