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

Allow RootElements and RootContainers to specify their node type #532

Closed
wants to merge 9 commits into from

Conversation

kcjonson
Copy link

  • Implements tagName attribute on RootElement and RootContainer that specifies what tag the element or container will render with.
  • Changes ClientController to search for any node type with matching data attribute (not just div)

Example usage:

getElements() {
    return <RootContainer tagName="section">
        <h2>Hello</h2>
        <RootElement className="content">
            <HelloWorld />
        </RootElement>
    </RootContainer>;
}

if (k !== 'tagName') {
root.setAttribute(k, v)
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This bit actually isn't necessary. The attributes we pass through are white listed in RootElement.js.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I hadn't gotten far enough... looks like it's being added as an attr there now. Hmm...

Copy link
Author

@kcjonson kcjonson Aug 11, 2016

Choose a reason for hiding this comment

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

Yea, its a kinda ugly bit of code. No harm arises by putting the attribute on the node, but its redundant information because the node with be of the type specified so I chose to exclude it.

@gigabo
Copy link
Contributor

gigabo commented Aug 12, 2016

I like this. It's going to be a cool improvement!

@gigabo gigabo added the enhancement New functionality. label Aug 12, 2016
@doug-wade
Copy link
Collaborator

🚀 i dig it

@gigabo
Copy link
Contributor

gigabo commented Aug 12, 2016

Hi @kcjonson, I just took another pass through here and realized that the root elements created during client transitions will also need to respect tagName. Take a look at _createReactServerRootNode and _createContainerNode in ClientController.js.

It would be good to have a test page set up to play around with this. You could either create a new page, or modify an existing page like the navigation playground to use non-div root containers/elements. That could actually really simplify the playground, which is table-like, but uses CSS on divs to lay it out. The other benefit there is you can use it to test all of the navigation variants (client transitions with and without DOM reuse, etc).

So, I think the remaining things I'd like to see here are:

  • Separate tagName from attrs
  • Support client transitions
  • Add a test page that uses tagName (or modify an existing page)

BTW - You'll probably want to pull and bootstrap to pick up #533 before you work on test pages.

@kcjonson
Copy link
Author

Good catch on the transition bug, will address that and moving the tagName out of attrs which was a dubiously good decision to put them there in the first place.

Will put some time into the playground too, as you know CSS/HTML is my jam.

@CLAassistant
Copy link

CLAassistant commented Aug 13, 2016

CLA assistant check
Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ kcjonson
❌ Kevin Jonson


Kevin Jonson seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.

@doug-wade
Copy link
Collaborator

@kcjonson looks like you may have committed as a different user than your github user -- maybe as your work user? Its possible adding another email address to your gh profile would resolve this, or git commit --amend --author

Copy link
Collaborator

@drewpc drewpc left a comment

Choose a reason for hiding this comment

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

@kcjonson I'm really looking forward to this change! Can you review the issues previously brought up and rebase from the master branch to ensure all tests work properly? We'd love to get this integrated but the PR has been sitting out there for a while!

@drewpc
Copy link
Collaborator

drewpc commented Jan 15, 2017

Closing this PR since there is no movement.

@drewpc drewpc closed this Jan 15, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New functionality.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants