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

Remove elementClasses warning from Fragment #375

Merged
merged 1 commit into from
Jun 11, 2018

Conversation

bryceosterhaus
Copy link
Member

I think this needs to be removed because it's not always consistent and really annoying when you intentionally want to have elementClasses added to the first child element. If anything, this should be added to documentation instead.

@jbalsas
Copy link
Contributor

jbalsas commented May 11, 2018

Hey @bryceosterhaus, I'm fine with this, just a bit confused about the statement

when you intentionally want to have elementClasses added to the first child element

How does that look like? As far as I understood Fragment was inert and rendered only its children... how does that look like in code and what's the expected (or current) output?

@bryceosterhaus
Copy link
Member Author

An example of intentionally wanting to add elementClasses to the first child would be. Fragment does only render its children, however, elementClasses will still get passed to the element attached to that component(compoenent.element). In the case below, <button> is the element attached to the component.

class Dropdown extends Component {
	render() {
		return (
			<Fragment>
				<button>Show Menu</button>

				{this.props.active && (
					<ul>
						<li>one</li>
						<li>two</li>
						<li>three</li>
					</ul>
				)}
			</Fragment>
		);
	}
}


<Dropdown elementClasses="blue-button" />

// renders
// <button class="blue-button">Show Menu</button>

@jbalsas
Copy link
Contributor

jbalsas commented May 14, 2018

I see, @bryceosterhaus!

So, for

<Dropdown elementClasses="blue-button" active={true} />

// We expect
// <button class="blue-button">Show Menu</button>
// <ul>
//    <li>one</li>
//    <li>two</li>
//    <li>three</li>
// </ul>

But if Dropdown implementation were to be reversed:

class Dropdown extends Component {
	render() {
		return (
			<Fragment>
				{this.props.active && (
					<ul>
						<li>one</li>
						<li>two</li>
						<li>three</li>
					</ul>
				)}

				<button>Show Menu</button>
			</Fragment>
		);
	}
}

You'd then expect

<Dropdown elementClasses="blue-button" active={true} />

// We expect
// <ul class="blue-button">
//    <li>one</li>
//    <li>two</li>
//    <li>three</li>
// </ul>
// <button>Show Menu</button>

<Dropdown elementClasses="blue-button" />

// We expect
// <button class="blue-button">Show Menu</button>

Does that match your expectations?

@bryceosterhaus
Copy link
Member Author

Yeah, that's what I would expect. I think we should note this in the Fragment docs.

@jbalsas
Copy link
Contributor

jbalsas commented May 14, 2018

I find this behaviour a bit confusing... 🤔

Maybe inside Fragment we could somehow expose it as props.elementClasses so the developer would actually clearly opt-in on how and where to use it?

@brunobasto, would this have any effect on you?

@brunobasto
Copy link

No, I think it should be fine for me.

@jbalsas
Copy link
Contributor

jbalsas commented May 14, 2018

@bryceosterhaus, @mthadley, as you guys are the main users of it, and @brunobasto has no objections at this point...

If you think it's fine as it is, could you please add a note of this behaviour in https://github.com/metal/metal.js/blob/master/packages/metaljs.com/src/pages/docs/guides/jsx-components.md#fragment- so we can have it documented at the time of merging?

Thanks!

@jbalsas jbalsas merged commit 2ab5593 into metal:develop Jun 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants