8000 [added] TabbedArea NavItem renderTab() className [fixes] #516 by thehuey · Pull Request #517 · react-bootstrap/react-bootstrap · GitHub
[go: up one dir, main page]

Skip to content

[added] TabbedArea NavItem renderTab() className [fixes] #516 #517

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 1 commit into from
Apr 27, 2015

Conversation

thehuey
Copy link
Contributor
@thehuey thehuey commented Apr 14, 2015

TabbedArea TabPane rendering doesn't carry across the className
property. This means it is not easy for users to customize
the look/feel of the tabs.

Added className to the properties copied in the renderTab function
and also pass additional properties down via ...other

@@ -108,12 +108,14 @@ const TabbedArea = React.createClass({
},

renderTab(child) {
let key = child.props.eventKey;
let {eventKey, className, tab, ...other} = child.props;
Copy link
Member

Choose a reason for hiding this comment

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

For what reason all ...other props could be applied from child to parent ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The variable 'child' is not accurate. It is the existing element being
rendered. In the 'render' function, it calls:

ValidComponentChildren.map(this.props.children, renderTabIfSet, this)

I think you are right and that I should remove this. It would copy the
TabPane properties to the corresponding Nav header.

On Tue, Apr 14, 2015 at 12:53 PM, Alexander Shemetovsky <
notifications@github.com> wrote:

In src/TabbedArea.js
#517 (comment)
:

@@ -108,12 +108,14 @@ const TabbedArea = React.createClass({
},

renderTab(child) {

  • let key = child.props.eventKey;
  • let {eventKey, className, tab, ...other} = child.props;

For what reason all ...other props could be applied from child to parent ?


Reply to this email directly or view it on GitHub
https://github.com/react-bootstrap/react-bootstrap/pull/517/files#r28363804
.

@mtscout6
Copy link
Member

Can you add some tests to ensure that classNames are getting applied as you expect? This way this feature is not accidentally broken in the future. Also, after you add some tests can you squash the commits. There's no need for multiple commits for this change.

@mtscout6
Copy link
Member

Closes #516

[added] TabbedArea NavItem renderTab() className

TabbedArea TabPane rendering doesn't carry across the className
property.  This means it is not easy for users to customize
the look/feel of the tabs.

Added className to the properties copied in the renderTab function
and also pass additional properties down via ...other
@thehuey thehuey force-pushed the tabbedarea-className branch from fa9f6a7 to 1d8b7c7 Compare April 22, 2015 19:58
@thehuey
Copy link
Contributor Author
thehuey commented Apr 22, 2015

@mtscout6 Test added to TabbedAreaSpec and squashed commits.

Once Travis CI finishes, I think this should be good.

@mtscout6
Copy link
Member

LGTM

/cc @react-bootstrap/collaborators How does this look?

@dozoisch
Copy link
Member

Just one quick question, this will apply the className passed to Tab on the NavItem. Not sure if it's the obvious behavior.

Should it be another prop like navClassName so that you can send different classNames to the NavItem and TabPane?

@mtscout6
Copy link
Member

@dozoisch Good call!

@thehuey
Copy link
Contributor Author
thehuey commented Apr 22, 2015

Currently there is no className being passed back to the renderPane function. @dozoisch If we want to support custom classes on either one, we should agree on a naming scheme and open a different ticket.

How do the following properties look:
className => Tab Pane, used in renderPane.
navClassName => NavItem class used in the Nav for tabs.

I'll open a new issue?

@dozoisch
Copy link
Member

@thehuey That looks good to me!

@mtscout6
Copy link
Member

The issue of the classNames is different, and will require some more discussion. Let's continue that in #560. I also updated the 1.0.0 roadmap documentation to address this issue.

mtscout6 added a commit that referenced this pull request Apr 27, 2015
[added] TabbedArea NavItem renderTab() className [fixes] #516
@mtscout6 mtscout6 merged commit 910668f into react-bootstrap:master Apr 27, 2015
@dozoisch dozoisch added this to the 1.0.0 Release milestone May 3, 2015
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.

4 participants
0