<KuiButton> needs submit prop

[bevacqua]

We're shadowing type='submit' for <button> because type is used to declare primary, secondary, and so on.

eui/src/components/button/button.js

Lines 74 to 77 in 52636ec

	<button
	disabled={isDisabled}
	className={classes}
	{...rest}

We should either rename type to something else such as hue='danger', or add a submit prop that results in <button type='submit'>.

Both approaches have their pros and cons. type fits the button "roles" very well, role would also clash with aria props, etc. At the same time, submit={ Boolean } wouldn't fix support for <button type='button'> either.

[snide]

There's also the third option of just making a new component that's more direct... ButtonSubmit or whatever. Not a great one either (cuz it'd mostly be a dupe), but that's how it's handled in the current code.

https://github.com/elastic/kibana/blob/master/ui_framework/src/components/button/button.js#L172-L187

[bevacqua]

@snide note that there's a bunch of different permutations, so that approach can be problematic:

• <input type='button'>
• <button>
• <button type='button'>
• <input type='submit'>
• <button type='submit'>

Probably what we need is to expose a function that given a set of parameters will give you the classes to style any of those elements, for the more advanced use cases. Something along the lines of:

export const getButtonClasses = ({ size, type }) =>
  `kuiButton kuiButton--${ size } kuiButton--${ type }`

[bevacqua]

@snide Here's an example from Cloud (there's quite a few cases like this, for normal buttons too) that would be a great consumer example for the above API:

const AddButton = ({ regionId, small = false, ...rest }) => <Link
  { ...rest }
  data-test-id='snapshotRepositories-addBtn'
  className={ cx(`btn btn--success`, { 'btn--sm': small }) }
  to={ createSnapshotRepositoryUrl(regionId) }>
  <FormattedMessage
    id='snapshot-repository-management.add-repository'
    defaultMessage='Add Repository' />
</Link>

With an API like the above, we could do something like

className={ getButtonClasses({ type: 'success', size: small ? 'small' : undefined }) }

[snide]

Yep. I agree, I don't think type would need to effect the presentation classes though luckily. I agree type makes better sense for the input type, and theme or even color works fine for the styling.

Normally I'd consider something like this a breaking change because it requires the devs to edit their existing work. We should probably talk a bit (later) about how we'd handle changes like this (release notes, versioning...etc) as we come across more of these.

[cjcenizal]

In Kibana master we changed type to buttonType to avoid this shadowing bug. I think we may have just overlooked this when we created this component in K7. So my suggestion is to rename this prop buttonType, and to apply the same naming pattern to other components which need a "type" property.

In terms of which combination of element and type attribute to support, I think we only need to support <button type="button"> and <button type="submit">. The input element doesn't provide any advantages over button, from what I've read. This would simplify implementation drastically, since we just need to stop shadowing the type prop and All Our Problems Are Magically Solved™.

WRT getButtonClasses, I think this implementation couples the prop values too tightly with the class names:

export const getButtonClasses = ({ size, type }) =>
  `kuiButton kuiButton--${ size } kuiButton--${ type }`

I agree that we'll want these values to be similar/identical to make the code easier to read, but I think decoupling them is just a good defensive programming practice to prevent stuff from accidentally breaking if we decide to tweak CSS class names or acceptable prop values. This also lets you see up-front which types are supported:

const buttonTypeToClassNameMap = {
  basic: 'kuiButton--basic',
  hollow: 'kuiButton--hollow',
  danger: 'kuiButton--danger',
  warning: 'kuiButton--warning',
  primary: 'kuiButton--primary',
  secondary: 'kuiButton--secondary',
};

const getClassName = ({ className, buttonType, hasIcon = false }) =>
  classNames('kuiButton', className, buttonTypeToClassNameMap[buttonType], {
    'kuiButton--iconText': hasIcon,
  });

[bevacqua]

Yeah, that's a great point. The implementation should definitely be whatever already exists in the components, and then we can expose those bits as a public API as well, so things that depend upon EUI can use the same logic without necessarily knowing about class names