Add helpertext to select #696
Conversation
| classes: this.theme(this.getRootClasses()) | ||
| }, labelAfter ? children.reverse() : children); | ||
| useNativeElement ? this.renderNativeSelect() : this.renderCustomSelect(), | ||
| w(HelperText, { theme, text: helperText }) |
There was a problem hiding this comment.
Would you want to conditionally render the helper text widget? Something like helperText ? w(HelperText, { theme, text: helperText }) : null
There was a problem hiding this comment.
I wanted to render it at all times. The helpertext has both a root and a conditional text node. The reasoning behind this was to allow the consumer to theme it to reserve it's space if they wish in order to stop the UI jumping around if they helpertext shows / hides. In reality, it's best practise to always show a helperText for a widget that will be validated.
There was a problem hiding this comment.
I guess the render possess is cheap enough where you don't need to worry about that then
tomdye
force-pushed
the
select-helper-text
branch
from
5d5f21d to
9ab46f2
Compare
Codecov Report
@@ Coverage Diff @@
## master #696 +/- ##
=========================================
Coverage ? 98.98%
=========================================
Files ? 45
Lines ? 3337
Branches ? 928
=========================================
Hits ? 3303
Misses ? 34
Partials ? 0
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #696 +/- ##
==========================================
- Coverage 99.04% 99.04% -0.01%
==========================================
Files 45 45
Lines 3344 3335 -9
Branches 927 926 -1
==========================================
- Hits 3312 3303 -9
Misses 32 32
Continue to review full report at Codecov.
|
| @@ -223,7 +225,8 @@ const expected = function(selectVdom: any, { classes = [ css.root, null, null, n | |||
| required: undefined, | |||
| forId: '' | |||
| }, [ 'foo' ]) : null, | |||
| selectVdom | |||
| selectVdom, | |||
| w(HelperText, { theme: undefined, text: helperText }) | |||
There was a problem hiding this comment.
Do you need to have theme as a property here?
There was a problem hiding this comment.
Don't we want to pass the actual theme through here?
There was a problem hiding this comment.
Urgh my bad, damn not using a top level test directory... that’s what I checked for 😂
Type: feature
The following has been addressed in the PR:
Description:
helperTextproperty toSelectlabelAfteras not compatible withhelperTextPart of: #677