Skip to content

Remove passing "all other" props to DOM nodes #773

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

Merged
merged 5 commits into from
Nov 15, 2016

Conversation

donnieberg
Copy link
Contributor

@donnieberg donnieberg commented Nov 11, 2016

This shouldn't change any functionality so if anyone sees anything, please let me know. I went through all the components and remove the {...props} from the DOM nodes (buttons, inputs, svg, etc).

I tried to think of all the common event handlers, attributes, and a11y stuff that might be passed down to these elements and added them manually. Please let me know what I've missed.
(Just an FYI for @tweettypography @chriscorwin @futuremint @iowillhoit)

In general, I wanted to ensure these props are passed down to <button>s (not inclusive list):

		const {
			ariaExpanded,
			ariaHaspopup,
			disabled,
			id,
			onBlur,
			onFocus,
			onKeyDown,
			onKeyPress,
			onKeyUp,
			onMouseEnter,
			onMouseLeave,
			tabIndex
		} = this.props;

And these props are passed down to form elements (input, checkbox - again, not all inclusive list):

const {
	ariaActivedescendant,
	ariaAutocomplete,
	ariaControls,
	ariaDescribedby,
	ariaExpanded,
	ariaOwns,
	ariaRequired,
	assistiveText,
	disabled,
	label,
	onBlur,
	onChange,
	onClick,
	onFocus,
	onInput,
	onInvalid,
	onKeyDown,
	onKeyPress,
	onKeyUp,
	onSelect,
	onSubmit,
        minLength,
	maxLength,
	name,
	placeholder,
	readOnly,
	required,
	role,
	type,
	value
} = this.props;

This PR also addresses:
Checkbox not responding to keyboard as it should - #753
Unknown props in stateful buttons - #417

@tweettypography tweettypography temporarily deployed to design-system-react-com-pr-773 November 11, 2016 21:57 Inactive
style={inputStyles}
type='text'
value={this.state.strValue}
/>
Copy link
Contributor Author

@donnieberg donnieberg Nov 11, 2016

Choose a reason for hiding this comment

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

The <input /> is the real area to focus on in this component ^

@donnieberg donnieberg force-pushed the removePassingPropsToDOM branch from c19425e to 5b7559b Compare November 11, 2016 22:16
@tweettypography tweettypography temporarily deployed to design-system-react-com-pr-773 November 11, 2016 22:16 Inactive
@interactivellama interactivellama added this to the 2016.11b-work-in-progress milestone Nov 14, 2016
@@ -75,6 +77,10 @@ const propTypes = {
* Deselect label and icon (optional) of button.
*/
stateThree: PropTypes.object,
/**
* Write <code>'-1'</code> if you don't want the user to tab to the button.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is parsed as markdown. Please change to "-1"

@minevskiy
Copy link
Contributor

Will SLDSButton prevent onMouseDown default? If not and it will bubble, I would add onMouseDown to the list.

Copy link
Contributor

@interactivellama interactivellama left a comment

Choose a reason for hiding this comment

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

Thanks for working on this tedious task. A few minor comments. I'll probably bump this up to 0.5.0 at release, since I'm 100% sure some of our engineers have hacked these attributes.

onInput,
pattern,
placeholder,
...props
Copy link
Contributor

Choose a reason for hiding this comment

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

This probably needs // eslint-disable-line no-unused-vars.

onMouseEnter: PropTypes.func,
onMouseLeave: PropTypes.func,
/**
* Write <code>"-1"</code> if you don't want the user to tab to the button.
Copy link
Contributor

Choose a reason for hiding this comment

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

"-1"

@interactivellama interactivellama changed the title Remove passing props to dom Remove passing "all other" props to DOM nodes Nov 14, 2016
@tweettypography tweettypography temporarily deployed to design-system-react-com-pr-773 November 14, 2016 22:23 Inactive
@donnieberg
Copy link
Contributor Author

@minevskiy good call -- I added onMouseDown to the button and stateful button.

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.

4 participants