feat: Readonly and inline ibm-label component for Input and TextArea #2466
Conversation
✅ Deploy Preview for carbon-components-angular ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
src/input/label.component.ts
Outdated
| @@ -148,6 +174,7 @@ export class Label implements AfterContentInit, AfterViewInit { | |||
| @ContentChild(TextArea, { static: false }) textArea: TextArea; | |||
|
|
|||
| @HostBinding("class.bx--form-item") labelClass = true; | |||
| @HostBinding("class.bx--text-input-wrapper") inputWrapperClass = true; | |||
There was a problem hiding this comment.
Is this intentional? It will be assigned to the host in both text area & text input or no matter what the content is 🤔
There was a problem hiding this comment.
It's what makes the inline variant work... I currently have it as a viable option for textarea as well.
There was a problem hiding this comment.
I understand that but if there is a change made to input wrapper, which causes visual issues with text area, we would have to make a change. So it is better to limit the wrappers to their respective children.
| @HostBinding("class.bx--text-input-wrapper") inputWrapperClass = true; | |
| @HostBinding("class.bx--text-input-wrapper") get inputWrapperClass() { | |
| return !this.textArea; | |
| } | |
| @HostBinding("class.bx--text-input--sm") get smallInputText() { | |
| return !this.textArea && this.size === "sm" && !this.inline; | |
| } | |
| @HostBinding("class.bx--text-input--md") get mediumInputText() { | |
| return !this.textArea && this.size === "md" && !this.inline; | |
| } | |
| @HostBinding("class.bx--text-input--xl") get largeInputText() { | |
| return !this.textArea && this.size === "xl" && !this.inline; | |
| } |
We don't want to bloat ibm-label component is used for more than just text input, in v11, it will be used for TextArea, TextInput, Password Input, There will be additional wrappers introduced for fluid styling.
There was a problem hiding this comment.
You were missing classes to assign size classes for normal Text input, so I'm including it with this PR. Ideally, we would want to have a separate feat PR for sizes but since it's part of inline as well, might as well include 🤷
There was a problem hiding this comment.
${prefix}--text-input--${size} is set in the ibmText directive:
https://github.com/carbon-design-system/carbon-components-angular/blob/master/src/input/input.directive.ts#L26-L34
anemonetea
force-pushed
the
textinput-feature-parity-2291-2292
branch
from
a02ad37
to
07072d2
Compare
|
Addressed comments:
|
src/input/label.component.ts
Outdated
| @@ -148,6 +174,7 @@ export class Label implements AfterContentInit, AfterViewInit { | |||
| @ContentChild(TextArea, { static: false }) textArea: TextArea; | |||
|
|
|||
| @HostBinding("class.bx--form-item") labelClass = true; | |||
| @HostBinding("class.bx--text-input-wrapper") inputWrapperClass = true; | |||
There was a problem hiding this comment.
I understand that but if there is a change made to input wrapper, which causes visual issues with text area, we would have to make a change. So it is better to limit the wrappers to their respective children.
| @HostBinding("class.bx--text-input-wrapper") inputWrapperClass = true; | |
| @HostBinding("class.bx--text-input-wrapper") get inputWrapperClass() { | |
| return !this.textArea; | |
| } | |
| @HostBinding("class.bx--text-input--sm") get smallInputText() { | |
| return !this.textArea && this.size === "sm" && !this.inline; | |
| } | |
| @HostBinding("class.bx--text-input--md") get mediumInputText() { | |
| return !this.textArea && this.size === "md" && !this.inline; | |
| } | |
| @HostBinding("class.bx--text-input--xl") get largeInputText() { | |
| return !this.textArea && this.size === "xl" && !this.inline; | |
| } |
We don't want to bloat ibm-label component is used for more than just text input, in v11, it will be used for TextArea, TextInput, Password Input, There will be additional wrappers introduced for fluid styling.
src/input/label.component.ts
Outdated
| }"> | ||
| <label | ||
| [for]="labelInputID" | ||
| [attr.aria-label]="ariaLabel" | ||
| class="bx--label" | ||
| [ngClass]="{ | ||
| 'bx--text-input__invalid-icon': !textArea, | ||
| 'bx--text-area__invalid-icon': textArea | ||
| 'bx--label--disabled': disabled, | ||
| 'bx--skeleton': skeleton, | ||
| 'bx--label--inline': inline, | ||
| 'bx--label--inline--sm': (inline && size === 'sm'), | ||
| 'bx--label--inline--md': (inline && size === 'md'), | ||
| 'bx--label--inline--xl': (inline && size === 'xl') | ||
| }"> | ||
| </svg> | ||
| <svg | ||
| *ngIf="!invalid && warn" | ||
| ibmIcon="warning--alt--filled" | ||
| size="16" | ||
| class="bx--text-input__invalid-icon bx--text-input__invalid-icon--warning"> | ||
| </svg> | ||
| <ng-content select="input,textarea,div"></ng-content> | ||
| <ng-content></ng-content> | ||
| </label> | ||
| <ng-template *ngIf="inline" | ||
| [ngTemplateOutlet]="helperTextTemplate" [ngTemplateOutletContext]="{inline: inline}"> | ||
| </ng-template> | ||
| </div> | ||
| <div | ||
| *ngIf="!skeleton && helperText && !invalid && !warn" | ||
| class="bx--form__helper-text" | ||
| [ngClass]="{'bx--form__helper-text--disabled': disabled}"> | ||
| <ng-container *ngIf="!isTemplate(helperText)">{{helperText}}</ng-container> | ||
| <ng-template *ngIf="isTemplate(helperText)" [ngTemplateOutlet]="helperText"></ng-template> | ||
| </div> | ||
| <div *ngIf="!warn && invalid" class="bx--form-requirement"> | ||
| <ng-container *ngIf="!isTemplate(invalidText)">{{invalidText}}</ng-container> | ||
| <ng-template *ngIf="isTemplate(invalidText)" [ngTemplateOutlet]="invalidText"></ng-template> | ||
| </div> | ||
| <div *ngIf="!invalid && warn" class="bx--form-requirement"> | ||
| <ng-container *ngIf="!isTemplate(warnText)">{{warnText}}</ng-container> | ||
| <ng-template *ngIf="isTemplate(warnText)" [ngTemplateOutlet]="warnText"></ng-template> | ||
| <div class="bx--text-input__field-outer-wrapper" | ||
| [ngClass]="{ | ||
| 'bx--text-input__field-outer-wrapper--inline': inline | ||
| }"> | ||
| <div | ||
| [class]="wrapperClass" | ||
| [ngClass]="{ | ||
| 'bx--text-input__field-wrapper--warning': warn | ||
| }" | ||
| [attr.data-invalid]="(invalid ? true : null)" | ||
| #wrapper> | ||
| <svg | ||
| *ngIf="!warn && invalid && !hasReadonlyStyle" | ||
| ibmIcon="warning--filled" | ||
| size="16" | ||
| [ngClass]="{ | ||
| 'bx--text-input__invalid-icon': !textArea, | ||
| 'bx--text-area__invalid-icon': textArea | ||
| }"> | ||
| </svg> | ||
| <svg | ||
| *ngIf="!invalid && warn && !hasReadonlyStyle" | ||
| ibmIcon="warning--alt--filled" | ||
| size="16" | ||
| class="bx--text-input__invalid-icon bx--text-input__invalid-icon--warning"> | ||
| </svg> | ||
| <svg | ||
| *ngIf="hasReadonlyStyle" | ||
| ibmIcon="edit--off" | ||
| size="16" | ||
| class="bx--text-input__readonly-icon"> | ||
| </svg> | ||
| <ng-content select="input,textarea,div"></ng-content> | ||
| </div> | ||
| <ng-container *ngIf="!inline" | ||
| [ngTemplateOutlet]="helperTextTemplate" [ngTemplateOutletContext]="{inline: inline}"> | ||
| </ng-container> | ||
| </div> | ||
|
|
||
| <ng-template #helperTextTemplate let-inline=inline> | ||
| <div *ngIf="!skeleton && helperText && ((!invalid && !warn) || inline)" | ||
| class="bx--form__helper-text" | ||
| [ngClass]="{ | ||
| 'bx--form__helper-text--disabled': disabled | ||
| }"> | ||
| <ng-container *ngIf="!isTemplate(helperText)">{{helperText}}</ng-container> | ||
| <ng-template *ngIf="isTemplate(helperText)" [ngTemplateOutlet]="helperText"></ng-template> | ||
| </div> | ||
| <div *ngIf="!inline && !warn && invalid" class="bx--form-requirement"> | ||
| <ng-container *ngIf="!isTemplate(invalidText)">{{invalidText}}</ng-container> | ||
| <ng-template *ngIf="isTemplate(invalidText)" [ngTemplateOutlet]="invalidText"></ng-template> | ||
| </div> | ||
| <div *ngIf="!inline && !invalid && warn" class="bx--form-requirement"> | ||
| <ng-container *ngIf="!isTemplate(warnText)">{{warnText}}</ng-container> | ||
| <ng-template *ngIf="isTemplate(warnText)" [ngTemplateOutlet]="warnText"></ng-template> | ||
| </div> | ||
| <ng-template> |
There was a problem hiding this comment.
We want to make this change in a way where we don't require existing CCA users to update their styles or make any changes. Changing order of label, adding wrapper classes will cause a breaking change for users that are making targeted style changes to label or input for example.
To counter this, we have to make use of templates... and a lot of em!
I've created the change for you with the comments, please go over them & let me know your thoughts! Additionally, after reviewing & working on this template update, I've come to the conclusion that having a shared wrapper with text area may not be the best idea for maintenance... even though it would be very similar. If we do keep a single wrapper for both TextArea & TextInput, the template complexity is going to increase again for v11 features such as fluid forms.
There is also Password Input which is going to be a separate component now. 😓
Please change the TEMPLATE to the following & remove the comments (<!-- -->), I've only added them to explain my grouping:
<!-- Use regular order for textarea & non inline -->
<ng-container *ngIf="!inline || textArea">
<ng-template [ngTemplateOutlet]="labelTemplate"></ng-template>
<ng-template [ngTemplateOutlet]="inputTemplate"></ng-template>
<ng-template [ngTemplateOutlet]="helperTextTemplate"></ng-template>
</ng-container>
<!-- If inline, we wrap in div & add additional classes -->
<ng-container *ngIf="inline && !textArea">
<div class="bx--text-input__label-helper-wrapper">
<ng-template [ngTemplateOutlet]="labelTemplate"></ng-template>
<ng-template [ngTemplateOutlet]="helperTextTemplate"></ng-template>
</div>
<div class="bx--text-input__field-outer-wrapper bx--text-input__field-outer-wrapper--inline">
<ng-container [ngTemplateOutlet]="inputTemplate"></ng-container>
</div>
</ng-container>
<!-- Template for label content -->
<ng-template #labelContentTemplate>
<ng-content></ng-content>
</ng-template>
<!-- Template for icons & input content -->
<ng-template #inputTemplate>
<div
[class]="wrapperClass"
[ngClass]="{
'bx--text-input__field-wrapper--warning': warn && !readonly && !textArea
}"
[attr.data-invalid]="(invalid ? true : null)"
#wrapper>
<svg
*ngIf="!warn && invalid && !hasReadonlyStyle"
ibmIcon="warning--filled"
size="16"
[ngClass]="{
'bx--text-input__invalid-icon': !textArea,
'bx--text-area__invalid-icon': textArea
}">
</svg>
<svg
*ngIf="!invalid && warn && !hasReadonlyStyle"
ibmIcon="warning--alt--filled"
size="16"
class="bx--text-input__invalid-icon bx--text-input__invalid-icon--warning">
</svg>
<svg
*ngIf="hasReadonlyStyle"
ibmIcon="edit--off"
size="16"
class="bx--text-input__readonly-icon">
</svg>
<ng-content select="input,textarea,div"></ng-content>
</div>
</ng-template>
<!-- Label template -->
<ng-template #labelTemplate>
<label
[for]="labelInputID"
[attr.aria-label]="ariaLabel"
class="bx--label"
[ngClass]="{
'bx--label--disabled': disabled,
'bx--skeleton': skeleton,
'bx--label--inline': inline,
'bx--label--inline--sm': inline && !textarea && size === 'sm',
'bx--label--inline--md': inline && !textarea && size === 'md',
'bx--label--inline--xl': inline && !textarea && size === 'xl'
}">
<ng-container *ngTemplateOutlet="labelContentTemplate"></ng-container>
</label>
</ng-template>
<!-- Helper Text & other state message template -->
<ng-template #helperTextTemplate>
<div *ngIf="!skeleton && helperText && ((!invalid && !warn) || inline)"
class="bx--form__helper-text"
[ngClass]="{
'bx--form__helper-text--disabled': disabled
}">
<ng-container *ngIf="!isTemplate(helperText)">{{helperText}}</ng-container>
<ng-template *ngIf="isTemplate(helperText)" [ngTemplateOutlet]="helperText"></ng-template>
</div>
<div *ngIf="!inline && !warn && invalid" class="bx--form-requirement">
<ng-container *ngIf="!isTemplate(invalidText)">{{invalidText}}</ng-container>
<ng-template *ngIf="isTemplate(invalidText)" [ngTemplateOutlet]="invalidText"></ng-template>
</div>
<div *ngIf="!inline && !invalid && warn" class="bx--form-requirement">
<ng-container *ngIf="!isTemplate(warnText)">{{warnText}}</ng-container>
<ng-template *ngIf="isTemplate(warnText)" [ngTemplateOutlet]="warnText"></ng-template>
</div>
<ng-template>There was a problem hiding this comment.
I've created another branch to test this out and put it in a PR (in my fork) for viewing the diffs: https://github.com/anemonetea/carbon-components-angular/pull/1/files
(From another comment) I agree that the outer wrapper shouldn't have been getting applied to in the textArea variant, so my branch fixes that issue.
Also agree that the shared ibm-label wrapper is making this code messy and it's not the best for maintenance...
We want to make this change in a way where we don't require existing CCA users to update their styles or make any changes. Changing order of label, adding wrapper classes will cause a breaking change for users that are making targeted style changes to label or input for example.
My concern with this approach is that we're further deviating from how React uses these CSS classes by not using bx--text-input__field-outer-wrapper for regular (not inline) instances of the textInput variant. So, if they change how the CSS works, our Angular stuff might break...
we don't require existing CCA users to update their styles or make any changes.
At least in my experience, my product team follows the wisdom of avoiding modifications of internal Carbon classes just in case Carbon makes changes. So, it's Carbon's prerogative to make changes to the internal working of Carbon components. They're also scared of updates because they might break stuff... so, every update needs to trigger regression testing. At the end of the day, it sounds like more of a matter of setting expectations about what kind of versioning change (fix, minor, major) can be expected to make changes to the internal DOM / CSS structure of components, and which ones can't -- not sure about how you guys handle this.
src/input/label.component.ts
Outdated
| @@ -148,6 +174,7 @@ export class Label implements AfterContentInit, AfterViewInit { | |||
| @ContentChild(TextArea, { static: false }) textArea: TextArea; | |||
|
|
|||
| @HostBinding("class.bx--form-item") labelClass = true; | |||
| @HostBinding("class.bx--text-input-wrapper") inputWrapperClass = true; | |||
There was a problem hiding this comment.
You were missing classes to assign size classes for normal Text input, so I'm including it with this PR. Ideally, we would want to have a separate feat PR for sizes but since it's part of inline as well, might as well include 🤷
src/input/label.component.ts
Outdated
| /** | ||
| * Set to `true` for a read-only label. | ||
| */ | ||
| @Input() @HostBinding("class.bx--text-input-wrapper--readonly") readonly = false; | ||
| /** | ||
| * Set to `true` for an inline style label. | ||
| */ | ||
| @Input() @HostBinding("class.bx--text-input-wrapper--inline") inline = false; |
There was a problem hiding this comment.
Turning these into getters/setters to better apply classes to host.
| /** | |
| * Set to `true` for a read-only label. | |
| */ | |
| @Input() @HostBinding("class.bx--text-input-wrapper--readonly") readonly = false; | |
| /** | |
| * Set to `true` for an inline style label. | |
| */ | |
| @Input() @HostBinding("class.bx--text-input-wrapper--inline") inline = false; | |
| /** | |
| * Set to `true` for a read-only label. | |
| */ | |
| protected _readonly = false; | |
| @Input() set readonly(enable: boolean) { | |
| this._readonly = enable; | |
| } | |
| get readonly() { | |
| return this._readonly; | |
| } | |
| @HostBinding("class.bx--text-input-wrapper--readonly") get isReadonly() { | |
| return this._readonly && !this.textArea; | |
| } | |
| /** | |
| * Set to `true` for an inline style label. | |
| */ | |
| protected _inline = false; | |
| @Input() set inline(enable: boolean) { | |
| this._inline = enable; | |
| } | |
| get inline() { | |
| return this._inline; | |
| } | |
| @HostBinding("class.bx--text-input-wrapper--inline") get isInline() { | |
| return this._inline && !this.textArea; | |
| } |
There was a problem hiding this comment.
I just want to point out that all of this extra logic still doesn't account for the case of a div being in the content of ibm-label:
<ng-content select="input,textarea,div">.
This all sounds like a bigger problem, if you ask me... Just the fact that carbon-ng uses the common label component whereas React doesn't (and we share the styles) -- it's not scalable. So, it is probably going to break down the line... and I'm afraid to even test the div version 😅 (there are no preexisting tests or storybooks for the div version...)
There was a problem hiding this comment.
Some notes about the current .bx--text-input-wrapper:
- never does anything on its own, only in combination with
-inlineclasses - relies on
.bx--form-itemto set thedisplay: flexfor the inline styling offlex-wrapto work
There was a problem hiding this comment.
tslint doesn't like the getter-setter code :(
ERROR: src/input/label.component.ts:159:2 - Declaration of public instance field not allowed after declaration of protected instance field. Instead, this should come after public static fields.
There was a problem hiding this comment.
I've changed this...
There was a problem hiding this comment.
I created a CodeSandbox with a bunch of different types of contents in ibm-label, including ones meeting the div selector: https://codesandbox.io/s/carbon-ng-inputs-1sf4f3?file=/src/app/app.component.html
- note that the
divversion ends up withbx--text-input__field-wrapper, and so does textArea for some reason...
There was a problem hiding this comment.
I've fixed the issue of the div case not being covered and the lifecycle issue that would be causing the text-area wrapper class to not show up in dynamic cases. (using ngAfterContentInit vs ngAfterContentChecked)
I've also added a Storybook for testing the dynamic case, but not sure if we'll want to keep this around permanently... I've mainly added it to aid in testing during review.
There was a problem hiding this comment.
warn modifier had the same issue as readonly and inline -- React textArea doesn't have the necessary styling to support it.
v10 has the class .#{$prefix}--text-input__field-wrapper--warning, but no such class for text-area.
- https://github.com/carbon-design-system/carbon/blob/v10/packages/styles/scss/components/form/_form.scss#L93
- React TextArea: https://github.com/carbon-design-system/carbon/blob/v10/packages/react/src/components/TextArea/TextArea.js
v11 introduces an analogous class -- .#{$prefix}--text-area__wrapper--warn:
anemonetea
force-pushed
the
textinput-feature-parity-2291-2292
branch
2 times, most recently
from
cc6f6fe
to
97b338d
Compare
- fix tests for ibm-label - add storybook example for template inputs to ibm-label Signed-off-by: anemonetea <laz1anastasiya@gmail.com>
Signed-off-by: anemonetea <laz1anastasiya@gmail.com>
Signed-off-by: anemonetea <laz1anastasiya@gmail.com>
…contents in ibm-label
anemonetea
force-pushed
the
textinput-feature-parity-2291-2292
branch
from
740401f
to
8a177fc
Compare
| class="bx--form__helper-text" | ||
| [ngClass]="{ | ||
| 'bx--form__helper-text--disabled': disabled, | ||
| 'bx--form__helper-text--inline': inline |
There was a problem hiding this comment.
I had missed adding this --inline class to helper text...
|
Hey @anemonetea - we recently added new input/textarea labels to make it easier to add input/textarea specific styling. Could you close this and remake the PR? It will be a lot easier to apply changes now. You will need to update Let me know if you have any questions and I can guide you through the changes. Feel free to reach out to me via slack! |
Closes:
#2291
Changing the
ibm-labelcomponent to add inputs forinlineandreadonlyoptions to match what is being done in React:Changelog
New
ibm-labelcontrolled by[inline]inputibm-labelcontrolled by[readonly]inputinputandtextareaelements added toibmTextandibmTextAreadirectives using[readonly]inputibm-labelhelper text inputsChanged
Removed
N/A