Skip to content
This repository has been archived by the owner on Mar 4, 2020. It is now read-only.

feat(input): new Input component with 'input' slot #326

Merged
merged 4 commits into from
Oct 18, 2018

Conversation

bmdalex
Copy link
Collaborator

@bmdalex bmdalex commented Oct 5, 2018

[RFC] Input

New input component according following the strategy in #314 (closes #314)
This PR contains:

  • new implementation of input (using input and wrapper slots);
  • Slot factory v1;
  • fix for input height in the chat prototype;
  • fix for tab keyboard navigation for radio group with inline input;
  • fixes for generic unit tests for shorthands;
  • lots of unit tests to cover Slot factory, input and wrapper slots for Input component, controlled/uncontrolled mode for Input, etc

TODO

  • Conformance test
  • Minimal doc site example
  • Stardust base theme
  • Teams Light theme
  • Teams Dark theme
  • Teams Contrast theme
  • Confirm RTL usage
  • W3 accessibility check
  • Stardust accessibility check
  • Update glossary props table
  • Update the CHANGELOG.md

Proposed solution

  • create an Input component consisting of an <input /> element wrapped with a <div />;
  • use partitionHTMLProps to decide what props (<Input {...props} />) go to the <input /> element or wrapping <div />;
  • use input slot to target the <input /> element and wrapper slot to target wrapping <div /> when partitionHTMLProps does not work as expected:
<Input {...partitionedProps} input={inputProps} wrapper={wrapperProps} />

e.g.:

<Input
  placeholder='Search...'
  dir='ltr'
  tabIndex={2}
  disabled={false}
  role='presentation'
  input={{ dir: 'rtl', tabIndex: 0 }}
  wrapper={{ disabled: true, role: 'checkbox' }}
/>

Input component has the following structure:

Props:
export interface IInputProps {
  as?: any
  className?: string
  clearable?: boolean
  defaultValue?: React.ReactText
  fluid?: boolean
  icon?: ShorthandValue
  inline?: boolean
  input?: ShorthandValue
  onChange?: ComponentEventHandler<IInputProps>
  renderIcon?: ShorthandRenderFunction
  renderInput?: ShorthandRenderFunction
  renderWrapper?: ShorthandRenderFunction
  styles?: ComponentPartStyle<IInputProps, any>
  type?: string
  value?: React.ReactText
  variables?: ComponentVariablesInput
  wrapper?: ShorthandValue
}
API
<Input {...inputProps} wrapper={wrapperProps} />

e.g.:

<Input
  placeholder="Search..."
  icon="search"
  clearable
  wrapper={{ role: "presentation" }}
/>
Component structure
<ElementType ...> // <div />
  <InputBase ... />  // <input />
  <Icon ...>  // <span />
</ElementType>
DOM structure
<div class="ui-input" role="presentation">
  <input type="text" placeholder="Search..." value="" />
  <span class="ui-icon" aria-hidden="true"></span>
</div>

This will solve all the scenarios mentioned above in the following fashion:

1. Styling the <input />
<Input
  placeholder="Search..."
  input={{ styles: { backgroundColor: 'red' height: '80px' } }}
/>
2. Setting HTML attributes for the <input />
<Input placeholder="Search..." input={{ tabIndex='-1' }} />
3. Setting HTML attributes for the wrapping <div />
<Input placeholder="Search..." wrapper={{ role: "presentation" }} />
Rendered component:

screen shot 2018-10-05 at 17 50 53

@bmdalex bmdalex self-assigned this Oct 5, 2018
@bmdalex bmdalex added the RFC label Oct 5, 2018
@bmdalex bmdalex mentioned this pull request Oct 8, 2018
@bmdalex bmdalex force-pushed the feat/input-new-component branch 2 times, most recently from 8a2fc91 to 723fda5 Compare October 8, 2018 15:49
@bmdalex bmdalex force-pushed the feat/input-new-component branch 3 times, most recently from a7241d4 to 4dad087 Compare October 9, 2018 08:38
@bmdalex bmdalex force-pushed the feat/input-new-component branch 6 times, most recently from 5686846 to d519ba8 Compare October 10, 2018 19:59
@bmdalex bmdalex force-pushed the feat/input-new-component branch 3 times, most recently from eeac1f1 to ad25278 Compare October 11, 2018 17:16
@bmdalex bmdalex removed the 🚧 WIP label Oct 11, 2018
@bmdalex
Copy link
Collaborator Author

bmdalex commented Oct 11, 2018

@levithomason @kuzhelov @miroslavstastny @mnajdova review needed

@codecov
Copy link

codecov bot commented Oct 11, 2018

Codecov Report

Merging #326 into master will increase coverage by 0.6%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #326     +/-   ##
=========================================
+ Coverage   89.33%   89.94%   +0.6%     
=========================================
  Files          64       64             
  Lines        1257     1253      -4     
  Branches      186      162     -24     
=========================================
+ Hits         1123     1127      +4     
+ Misses        131      123      -8     
  Partials        3        3
Impacted Files Coverage Δ
src/components/Input/Input.tsx 100% <100%> (+15.38%) ⬆️
src/components/Slot/Slot.tsx 100% <100%> (ø) ⬆️
src/index.ts 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update be50fab...93156c3. Read the comment docs.

it('sets correct "as" prop in defaultProps', () => {
const as = 'span'
const options = { testOption: 'test option value' }
const createShorthandSpy = spyOn(lib, 'createShorthand')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here we are checking the implementation details - we should rather check method's result instead

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed with @kuzhelov to add the tests in the dedicated PR for Slot factory that's about to come

const createShorthandSpy = spyOn(lib, 'createShorthand')
createSlotFactory(as, () => ({}))('testValue', { defaultProps: { as: asOverride } }) // clone of the options

const optionsArg = createShorthandSpy.calls.mostRecent().args[3]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this test is very brittle - if the next day we will check the arguments that createShorthand accepts, this will fail this test - while this failure won't surely guarantee that the logic of createSlotFactory has become to be incorrect

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed with @kuzhelov to add the tests in the dedicated PR for Slot factory that's about to come

Copy link
Collaborator Author

@bmdalex bmdalex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

addressed @kuzhelov 's comments

Note: Please provide all the initial feedback on the first review so I can address it from beginning. It'd be better not to start new conversations in the phase of addressing review comment (after initial code review from specific dev) unless there are code changes in that area, of course

docs/src/examples/components/Input/Variations/index.tsx Outdated Show resolved Hide resolved
@@ -21,6 +21,7 @@ class ComposeMessage extends React.Component {
<Input
fluid
placeholder="Type a message"
input={{ styles: { height: '44px' } }}
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, done, thx

test/specs/commonTests/implementsWrapperProp.tsx Outdated Show resolved Hide resolved
test/specs/commonTests/implementsWrapperProp.tsx Outdated Show resolved Hide resolved
test/specs/commonTests/implementsWrapperProp.tsx Outdated Show resolved Hide resolved
test/specs/commonTests/implementsWrapperProp.tsx Outdated Show resolved Hide resolved
src/components/Input/Input.tsx Outdated Show resolved Hide resolved
@kuzhelov kuzhelov changed the title feat(input): new input component feat(input): new Input component with 'input' slot Oct 18, 2018
@bmdalex bmdalex force-pushed the feat/input-new-component branch 3 times, most recently from 0d442e6 to 78503f9 Compare October 18, 2018 19:47
@bmdalex bmdalex merged commit 3d7cb18 into master Oct 18, 2018
@bmdalex bmdalex deleted the feat/input-new-component branch October 18, 2018 20:16
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[RFC] New Input component
5 participants