Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(Input): update to v1 API #281

Merged
merged 25 commits into from
Sep 28, 2016

Conversation

jhchill666
Copy link
Contributor

@jhchill666 jhchill666 commented Jun 24, 2016

Fixes #291

@codecov-io
Copy link

codecov-io commented Jun 24, 2016

Current coverage is 99.49% (diff: 100%)

Merging #281 into master will increase coverage by 0.50%

@@             master       #281   diff @@
==========================================
  Files           112        113     +1   
  Lines          1767       1768     +1   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           1749       1759    +10   
+ Misses           18          9     -9   
  Partials          0          0          

Powered by Codecov. Last update ab4f7ba...e27f533

@levithomason
Copy link
Member

Input API Proposal

Here is how I see the v1 API applying to the Input. Would love feedback. I'll update this comment as we narrow things down in the comments.

Input

<Input />
<div class="ui input">
  <input type="text" >
</div>

Focus

<Input focus />
<div class="ui input focus">
  <input type="text">
</div>

Loading

In SUI, the loading style automatically adds a spinner icon, or overrides the provided icon. It then requires either an icon or left icon class to correctly place the icon on the left or right.

Loading only

I propose we add the icon class by default when the loading prop is given. This would place the spinner on the right inside of the input.

<Input loading />
<div class="ui icon input loading">
  <input type="text">
</div>

Disabled

<Input disabled />
<div class="ui disabled input">
  <input type="text" disabled="disabled">
</div>

Error

<Input error />
<div class="ui input error">
  <input type="text">
</div>

Icon

I see two possible APIs for the input icons:

  1. Makes use of the iconPropRenderer
  2. Follows the labeled and action pattern (ie icon or icon='left') and allows an Icon as an input child.

The two suggestions here follow the first option above. After writing the rest of the examples, I am leaning toward API number 2 (see alternative below).

Icon

<Input icon='search' />
<div class="ui icon input">
  <input type="text">
  <i class="search icon"></i>
</div>

Left Icon

We might expect this to be icon='left' however, there is then no way to pass the icon string. Open to more ideas on how to achieve left/right icon positions.

<Input leftIcon='search' />
<div class="ui left icon input">
  <input type="text">
  <i class="users icon"></i>
</div>

Alternative

The alternative is to allow Icons as children (as we do with actions and labels). The Input icon class would be inferred if there is an Icon child.

It would exclude the use of the iconPropRenderer. Therefore excluding the simple icon='search' use as well.

Though it would be more consistent with the labeled an action props. Lastly, it follows the cx utilities as well icon='left' => class="left icon". Whereas, iconLeft='search' seems to break away from the pattern.

I think I prefer this alternative syntax to the icon prop but would love any feedback.

// same as <Input icon>
<Input>
  <Icon name='user' />
</Input>

<Input icon='left'>
  <Icon name='settings' />
</Input>

<Input action='right' icon='left'>
  <Icon name='search' />
  <Button>Search</Button>
</Input>
<div class="ui icon input">
  <input type="text">
  <i class="user icon"></i>
</div>

<div class="ui left icon input">
  <input type="text">
  <i class="settings icon"></i>
</div>

<div class="ui left icon right action input">
  <i class="search icon"></i>
  <input type="text">
  <button>Search</button>
</div>

Labeled

Labeled Inputs can be pretty complex. I've include all the SUI examples here to be sure we cover them all.

Our current Input supports most all of these already. The approach was to use children for any buttons, dropdowns, or labels.

Again, any input or feedback is much welcomed.

Label

Just as I am proposing the loading input automatically adding the icon class, I propose a Label child automatically adds the labeled style.

// same as `<Input labeled>`, inferred by child
<Input>
  <Label>http://<Label>
</Input>
<div class="ui labeled input">
  <div class="ui label">http://</div>
  <input type="text">
</div>

Label Dropdown

The docs for this input configuration show use of a label class on the dropdown. This is not documented in the dropdown itself. I've added #291 to consider how we should support it.

<Input labeled='right'>
  <Dropdown label options={[...]} />
</Input>
<div class="ui right labeled input">
  <input type="text">
  <div class="ui dropdown label">
    <div class="text">.com</div>
    <i class="dropdown icon"></i>
    <div class="menu">
      <div class="item">.com</div>
      <div class="item">.net</div>
      <div class="item">.org</div>
    </div>
  </div>
</div>

Right Label

<Input labeled='right'>
  <Label basic>kg<Label>
</Input>
<div class="ui right labeled input">
  <input type="text">
  <div class="ui basic label">kg</div>
</div>

Left and Right Label

When two labels are given, the input must be in between them in the markup. Notice also the HTML requires the right labeled class even though there are labels on both sides.

I propose the order of the label children should determine which is left/right. Also, that the style labeled='right' should be inferred by the presence of two labels.

// same as <Input labeled='right' />
<Input>
  <Label basic>$<Label>
  <Label basic>kg<Label>
</Input>
<div class="ui right labeled input">
  <div class="ui label">$</div>
  <input type="text">
  <div class="ui basic label">.00</div>
</div>

Left Corner Labeled

Corner labeled inputs require HTML classes that match the corner position of the Label itself. I propose we infer the Input classes from the Label props.

// same as <Input labeled='left corner' />
<Input>
  <Label corner='left'>
    <Icon name='asterisk' />
  <Label>
</Input>
<div class="ui left corner labeled input">
  <input type="text">
  <div class="ui left corner label">
    <i class="asterisk icon"></i>
  </div>
</div>

Corner Labeled

As with the left corner labeled Input above, I propose we infer the Input labeled classes to match the label props.

See #292 for Label boolean corner prop support

// same as <Input labeled='corner' />
<Input>
  <Label corner>
    <Icon name='asterisk' />
  <Label>
</Input>
<div class="ui corner labeled input">
  <input type="text">
  <div class="ui corner label">
    <i class="asterisk icon"></i>
  </div>
</div>

Action

I see the action prop behaving much like the labeled prop. Since there are only a limited number of supported Input actions, I think we should infer the action prop settings for the Input.

I also notice action class is synonymous to right action.

Action

// same as <Input action />
<Input>
  <Button>Search<Button>
</Input>
<div class="ui action input">
  <input type="text">
  <button class="ui button">Search</button>
</div>

Left Action

<Input action='left'>
  <Button color='teal' labeled icon='cart'>
    Search
  <Button>
</Input>
<div class="ui left action input">
  <button class="ui teal labeled icon button">
    <i class="cart icon"></i>
    Checkout
  </button>
  <input type="text" value="$52.03">
</div>

Right Action with Left Icon

This shows the alternative form of the proposed icon API

<Input action='right' icon='left'>
  <Icon name='search' />
  <Dropdown basic floating button options={[...]} />
</Input>
<div class="ui right action left icon input">
  <i class="search icon"></i>
  <input type="text">
  <div class="ui basic floating dropdown button">
    <div class="text">This Page</div>
    <i class="dropdown icon"></i>
    <div class="menu">
      <div class="item">This Organization</div>
      <div class="item">Entire Site</div>
    </div>
  </div>
</div>

Dropdown Action

// same as <Input action>, inferred by children
<Input>
  <Dropdown compact selection options={[...]} />
  <Button type='submit'>Search</Button>
</Input>
<div class="ui action input">
  <input type="text">
  <select class="ui compact selection dropdown">
    <option value="all">All</option>
    <option selected="" value="articles">Articles</option>
    <option value="products">Products</option>
  </select>
  <div type="submit" class="ui button">Search</div>
</div>

Transparent

<Input transparent />
<div class="ui transparent input">
  <input type="text">
</div>

Inverted

<Input inverted />
<div class="ui inverted input">
  <input type="text">
</div>

Fluid

<Input fluid />
<div class="ui fluid icon input">
  <input type="text">
</div>

Size

<Input size='mini' />
<Input size='small' />
<Input size='large' />
<Input size='big' />
<Input size='huge' />
<Input size='massive' />
<div class="ui mini icon input">
  <input type="text">
</div>

<div class="ui small icon input">
  <input type="text">
</div>

<div class="ui large icon input">
  <input type="text">
</div>

<div class="ui big icon input">
  <input type="text">
</div>

<div class="ui huge icon input">
  <input type="text">
</div>

<div class="ui massive icon input">
  <input type="text">
</div>

@jhchill666
Copy link
Contributor Author

jhchill666 commented Jun 27, 2016

I've been having a look at labeled/action syntax and seems like a bit of a stumbling point, not only from an implementation standpoint, but also one of user clarity.

Leaving Icon out, how many labels/actions can an Input feasibly have? Is it correct to assume one of either, one left and one right? Assuming there is some limit, how do you handle other added components that don't fit the template?

I'm not a fan of making user assumptions, as more often than not they turn out to be incorrect. Is there no way of ring fencing left/right label/action's and to follow the emerging syntax a little more closely? (just thinking out loud here):

1 Delegate responsibility for positioning to sub-components

<Input>
  <Input.Left icon="search" />
  <Input.Right action>
    <Dropdown compact selection options={[...]} />
  </Input.Right>
</Input>

2 Delegate responsibility for behavior to sub-components

<Input>
  <Input.Labeled left icon="search" />
  <Input.Action right>
    <Button type='submit'>Search</Button>
  </Input.Action>
</Input>

3 Delegate auxillary components to sub-components

<Input>
  <Input.Label left icon="search" />
  <Input.Button right>Search</Input.Button>
</Input>

?

}
})

const classes = cx('sd-input ui',
Copy link
Member

Choose a reason for hiding this comment

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

Heads up, I've finished removing all sd-* classes, see #301. Rebase to master to get the update. The test for the sd-* class has been inverted to fail if any sd- class exists.

Let's remove all sd-* classes from all PRs. 👍

@levithomason levithomason modified the milestone: v1.0 Jun 30, 2016
@levithomason levithomason changed the title Input Component updated to v1 API Input: update to v1 API Aug 18, 2016
@levithomason
Copy link
Member

I've got an updated proposal that I intend to post here once I finish up the Form in #400. My focus will then shift to Input and Button since they are very related.

@levithomason
Copy link
Member

Heads up! I'm resuming work on this PR now 🍻

@jeffcarbs
Copy link
Member

jeffcarbs commented Sep 10, 2016

@levithomason - I have a request/idea for this component: It's currently not possible to add props to the underlying input that are also props of the Input component (e.g. there's no way to add a class to the input since className is a handled prop of Input. This is preventing usage of the Input component within the Search component since the input tag needs the prompt class.

I'd propose an input shorthand prop that is passed to a factory to create the <input> tag. A string could be treated at the type, options would be passed as props. The defaultProp would be text so you'd get the current default (<input type="text">).

The weird thing here would be that you could technically pass null to not render an <input> tag at all, although I can't see any scenario where anyone would do that unintentionally.

@levithomason
Copy link
Member

Cool, I'll incorporate an input shorthand here.

@levithomason levithomason force-pushed the feature/input branch 3 times, most recently from f9999d1 to 8f4d24b Compare September 19, 2016 18:33
@levithomason levithomason changed the title Input: update to v1 API feat(Input): update to v1 API Sep 19, 2016
{isLeftLabeled && labelChildren}
{isLeftAction && actionChildren}
<input {...inputProps} type={type} />
{icon && <Icon name={icon} />}
Copy link
Member

@jeffcarbs jeffcarbs Sep 20, 2016

Choose a reason for hiding this comment

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

This should use the createIcon factory.

Copy link
Member

Choose a reason for hiding this comment

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

Indeed, will update.

useKeyOnly(inverted, 'inverted'),
useKeyOnly(loading, 'loading'),
useKeyOnly(transparent, 'transparent'),
icon && 'icon',
Copy link
Member

Choose a reason for hiding this comment

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

Should this be useKeyOnly? Or is that reserved for boolean props?

Copy link
Member

Choose a reason for hiding this comment

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

Should be, thanks.

@levithomason
Copy link
Member

Update

The newly updated API spec for customizing the HTML <input /> and dealing with the labeled and action variations this looks like so:

<Input
  label='http://'
  labelPosition='left'
  input={<input />}
  action={<Button icon='heart' content='Like' />}
  actionPosition='right'
/>

Where label, input, and action are all shorthand props. They will accept a literal, props object, or element. Also note, any input props passed are automatically spread on the <input /> itself. So the input shorthand prop here is mainly needed for adding classes / style props to the input.

This will be my main focus tomorrow. The Button API is scheduled to be updated to keep consistent. See #510.

@levithomason
Copy link
Member

levithomason commented Sep 22, 2016

The Input API spec of <Input label='.com' labelPosition='right' /> will not work for inputs with a label on the left and on the right:

image

The same actions API <Input action='Checkout' actionPosition='left' /> will not work for inputs with multiple actions:

image

The first thought was to go with leftLabel rightLabel, however, this does not extend to the actions very well. There can be two actions on one side. Ideally, the solution would be consistent for icons, labels, and actions and for all components. Here is a start at the available patterns for each prop that will work with all permutations:

icon iconPosition

Works for all components, only ever one icon

leftLabel rightLabel

Works for all components, only ever one label per side

actions actionsPosition

If actions took an array of components, this would work. can have multiple actions, but only ever on one side at a time

What I don't like is the inconsistency here. I'd almost rather support a single icon, label, or action and go with foo fooPosition across the board.

@jcarbo @layershifter Help? 😕

@layershifter
Copy link
Member

layershifter commented Sep 22, 2016

What about this?

const elements = [
  <Label>Left</Label>,
  {position: 'left', element: <Label>Left</Label>},
  {position: 'right', element: <Label>Rigth</Label>},
];

function Input () {
  const leftElements = _map(elements, element => {
    if(_.isObject(element) && element.position === 'right') return
    return element
  })

  const rightElements = _map(elements, element => {
    if(_.isObject(element) && element.position === 'right') {
      return element
    }
  })

  return (
    <div>
      {leftElements}
      <input>
      {rightElements}
   </div>
  )
}

const myInput = <Input elements={elements} />

It will be simply validatable with props, but there will be key problem.

@levithomason
Copy link
Member

It is an option, I actually suggested this same thing to our team offline :)

<Input labels={[
  { position: 'left', content:'$' },
  { position: 'right', content: '.00', basic: true },
]} />

It just seems too verbose / clunky IMO. That said, it is better than having a different API for every prop. I'm not sure if it is better than just supporting a single icon/label/action with a single foo``fooPosition API though.

@levithomason
Copy link
Member

Ah! Long awaited, but I think it is finally done.

Copy link
Member

@levithomason levithomason left a comment

Choose a reason for hiding this comment

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

@jcarbo @layershifter this is ready for final. If either of you could scan the code or pull this branch and try it out, I'd much appreciate it. Thanks!

}

componentWillUnmount() {
document.removeEventListener('keydown', this.handleDocumentKeyDown)
}

setSearchInput() {
this._searchInput = findDOMNode(this).querySelector('.ui.input input')
Copy link
Member

Choose a reason for hiding this comment

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

Input is now a stateless component, so no refs are available. Updated the doc sidebar to use a regular query selector instead.

}

const actionElement = Button.create(action, elProps => ({
Copy link
Member

Choose a reason for hiding this comment

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

Sometimes the defaultProps set by a shorthand factory are conditional based on the elements props. Updated factory default props to take an object or a function that receives the props and returns the defaultProps.

@@ -571,7 +572,8 @@ export const implementsShorthandProp = (Component, options = {}) => {
ShorthandComponent,
mapValueToProps,
requiredProps = {},
requiredShorthandProps = {},
shorthandDefaultProps = {},
alwaysPresent,
Copy link
Member

Choose a reason for hiding this comment

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

Added an alwaysPresent flag to shorthand props. This means it is not tested for "does not exist when null" and other cases. The Input always has a shorthand input rendered.

}))
const iconElement = Icon.create(icon)
const labelElement = Label.create(label, elProps => ({
className: cx(
Copy link
Member

Choose a reason for hiding this comment

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

Any reason for not using what cx offers here?

className: cx({
  label: !_includes(...),
  [labelPosition]: _.includes(...),
})

Copy link
Member

Choose a reason for hiding this comment

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

In early tests, the object form was 8-10x slower than the short circuited argument approach. This is one reason we went with the lib utils we have for className buildup. The timing difference will probably never be an issue. I just continue it out of consistency and habit.

@@ -30,23 +30,39 @@ const mergePropsAndClassName = (defaultProps, props) => {
* @param {function|string} Component A ReactClass or string
* @param {function} mapValueToProps A function that maps a primitive value to the Component props
* @param {string|object|function} val The value to create a ReactElement from
* @param {object} defaultProps Default props to add to the final ReactElement
* @param {object|function} [defaultProps] Default props object or function (called with regular props).
Copy link
Member

Choose a reason for hiding this comment

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

Is there a clearer term that we could use rather than regular props?

Copy link
Member

Choose a reason for hiding this comment

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

I struggled with that, suggestions welcome. It is one of 3 props. The props from the element the user passed, a props object the user passed, or the props generated by the factory's mapValueToProps using string/number literal the user passed.

@@ -184,7 +187,7 @@ export default class Dropdown extends Component {

inline: PropTypes.bool,
labeled: PropTypes.bool,
linkItem: PropTypes.bool,
// linkItem: PropTypes.bool,
Copy link
Member

Choose a reason for hiding this comment

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

cruft?

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the careful eye. Intentionally disabled and will probably be removed entirely when the rest of the Dropdown examples are completed.

This PR sets a precedent for how we deal with components that require special classNames only when used in combinatory contexts (Input with action component). This was done with the Input action and label. Both of which should always have an action or label className respectively.

In the factory we can add classNames:

Button.create(val, { className: 'action' })

Or, we can use augmentation, which merges classNames:

<Dropdown as={Menu.Item} />

@@ -903,7 +907,7 @@ export default class Dropdown extends Component {
icon,
inline,
labeled,
linkItem,
// linkItem,
Copy link
Member

Choose a reason for hiding this comment

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

^^

@@ -935,7 +940,7 @@ export default class Dropdown extends Component {
// useKeyOnly(icon, 'icon'),
useKeyOnly(labeled, 'labeled'),
// TODO: linkItem is required only when Menu child, add dynamically
useKeyOnly(linkItem, 'link item'),
// useKeyOnly(linkItem, 'link item'),
Copy link
Member

Choose a reason for hiding this comment

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

^^


import { Message } from 'src'

const InputStates = () => (
Copy link
Member

Choose a reason for hiding this comment

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

Var name might be States

Copy link
Member

Choose a reason for hiding this comment

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

In order to aide in searching, let's always keep the component name in the default export name. Let's also keep the filename the same as the default export (except for index.js files of course).

This way the default export matches the filename and we can easily search for either. This makes automated refactors much easier to work with. Once we're done with the legacy components I plan to update all doc files to be consistent.

import ComponentExample from 'docs/app/Components/ComponentDoc/ComponentExample'
import ExampleSection from 'docs/app/Components/ComponentDoc/ExampleSection'

const InputTypes = () => (
Copy link
Member

Choose a reason for hiding this comment

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

^^

import { Input, Label } from 'stardust'

const InputRightLeftLabeled = () => (
<Input labelPosition='right' placeholder='mysite.com'>
Copy link
Member

Choose a reason for hiding this comment

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

labelPosition='right' is cruft there?

Copy link
Member

Choose a reason for hiding this comment

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

I wish :( The input has some very odd markup. I think it needs some core SUI work to make more sense. The labeled class alone formats the left label. While adding right labeled also formats the right label, but the labeled portion of that class still formats the left label also. Very confusing and unfortunate.

http://semantic-ui.com/elements/input.html#labeled

image


import { Message } from 'src'

const InputVariations = () => (
Copy link
Member

Choose a reason for hiding this comment

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

^^

if (_.isPlainObject(val)) {
return <Component {...mergePropsAndClassName(defaultProps, val)} />
defaultProps = _.isFunction(defaultProps) ? defaultProps(usersProps) : defaultProps
const props = mergePropsAndClassName({ ...defaultProps }, usersProps)
Copy link
Member

Choose a reason for hiding this comment

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

I don't see any reason to spread defaultProps here. This could just be:

const props = mergePropsAndClassName(defaultProps, usersProps)

Copy link
Member

Choose a reason for hiding this comment

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

Ah, looks like defaultProps can be undefined, in which case that was creating an empty object so mergePropsAndClassName didn't throw.

I've instead just defaulted defaultProps = {} and removed the spread for clarity.

@levithomason levithomason merged commit 38cb682 into Semantic-Org:master Sep 28, 2016
@levithomason levithomason deleted the feature/input branch September 28, 2016 17:27
@levithomason
Copy link
Member

Whew, released in stardust@0.48.0! Thanks to everyone for chipping in on this one in so many ways. Much appreciated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants