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

[DirectionProvider] Allow passing additional props to DirectionProvider #17

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
2 participants
@TaeKimJR
Copy link

commented May 7, 2019

This PR adds the ability to pass additional props (...rest) to the Tag rendered by the DirectionProvider. In addition, it passes additional props from the AutoDirectionProvider to the DirectionProvider.

The goal of this PR is to reduce the number of nodes rendered on any given page. If we can re-use the DOM node created by the Direction Provider, we can reduce nesting by 1.

@@ -45,10 +44,10 @@ export default class DirectionProvider extends React.Component {
}

render() {
const { children, direction, inline } = this.props;
const { children, direction, inline, ...rest } = this.props;

This comment has been minimized.

Copy link
@ljharb

ljharb May 7, 2019

Member

instead of ...rest, at the least, could this be an actual tagProps prop or something?

}) {
const textDirection = getDirection(text);
const dir = textDirection === 'neutral' ? direction : textDirection;

return (
<DirectionProvider
{...rest}

This comment has been minimized.

Copy link
@ljharb

ljharb May 7, 2019

Member

we've generally tried to avoid doing this; spreading props is generally an antipattern :-/ there's no explicit propType warnings, so invalid DOM props end up only being runtime warnings.

This will also allow people to pass style and className props, which we've explicitly gone out of our way to avoid allowing, per https://brigade.engineering/don-t-pass-css-classes-between-components-e9f7ab192785

This comment has been minimized.

Copy link
@TaeKimJR

TaeKimJR May 7, 2019

Author

Ah, that was my intention haha. I wanted to re-use this DOM node as a wrapper <div>.

I was kind of iffy about this PR to begin with, but I think you've reminded me of a good point. Thank you, I will close this PR.

@TaeKimJR

This comment has been minimized.

Copy link
Author

commented May 7, 2019

Closing in favor of good practice!

@TaeKimJR TaeKimJR closed this May 7, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.