Remove text-input Addon widget
#1697
Conversation
|
This pull request is being automatically deployed with Vercel (learn more). widget-test-docs – ./🔍 Inspect: https://vercel.com/dojo/widget-test-docs/Gy92jP8wXeaM1hngYtN56vyugqDk dojo.widgets – ./🔍 Inspect: https://vercel.com/dojo/dojo.widgets/CyftXDNLsmxAsevtD41My821D5Va |
Codecov Report
@@ Coverage Diff @@
## master #1697 +/- ##
==========================================
- Coverage 90.05% 90.05% -0.01%
==========================================
Files 94 94
Lines 5050 5046 -4
Branches 1373 1374 +1
==========================================
- Hits 4548 4544 -4
Misses 249 249
Partials 253 253
Continue to review full report at Codecov.
|
src/theme/dojo/text-input.m.css
Outdated
| color: var(--color-text-faded); | ||
| flex: 0 0 auto; | ||
| font-size: inherit; | ||
| line-height: var(--line-height-base); | ||
| padding: 0 var(--grid-base); | ||
| /* padding: 0 var(--grid-base); */ |
There was a problem hiding this comment.
Did you mean to change this value here?
|
Looks good Tom! What was the original thought behind the |
| @@ -606,12 +610,12 @@ registerSuite('TextInput', { | |||
| css.noLabel | |||
| ]) | |||
| .prepend('@inputWrapper', () => [leading]); | |||
| const h = harness(() => <TextInput>{{ leading }}</TextInput>); | |||
| const h = harness(() => <TextInput>{{ leading: <span>A</span> }}</TextInput>); | |||
There was a problem hiding this comment.
You could probably get away with just using a text node instead of a full span.
| const h = harness(() => <TextInput>{{ leading: <span>A</span> }}</TextInput>); | |
| const h = harness(() => <TextInput>{{ leading: 'A' }}</TextInput>); |
There was a problem hiding this comment.
yup yup, you are correct thought wanted to show that you can add domnodes as well as text
| @@ -11,8 +10,8 @@ export default factory(function Basic() { | |||
| <NumberInput> | |||
| {{ | |||
| label: 'Number Input', | |||
| leading: <Addon>$</Addon>, | |||
| trailing: <Addon filled>MM</Addon> | |||
| leading: <span>$</span>, | |||
There was a problem hiding this comment.
I think we should show users that they can pass text here. For most use cases that should be sufficient.
| leading: <span>$</span>, | |
| leading: '$', |
There was a problem hiding this comment.
agreed, though I have shown a mix of uses.
I believe it was to make the padding optional but this can be done better via theming etc now. The previous approach was awkward for custom elements. |
|
@aciccarello comments addressed |
This PR removes the text-input
Addonwidget and makes it more user friendly.You can now simply pass the leading / trailing dom node directly to the
TextInput.Resolves: #1695