Skip to content

fix(secondary-tabs): refactor the secondary tabs into nested structure#501

Closed
dabeng wants to merge 1 commit intopatternfly:masterfrom
dabeng:sec-tabs
Closed

fix(secondary-tabs): refactor the secondary tabs into nested structure#501
dabeng wants to merge 1 commit intopatternfly:masterfrom
dabeng:sec-tabs

Conversation

@dabeng
Copy link
Contributor

@dabeng dabeng commented Jan 11, 2018

This PR is targeted to fix issue.

Please review the Demo.

@mcarrano
Copy link
Member

@dabeng @jgiardino I tried to follow the discussion of this in the React repo, but have to admit to having a hard time understanding what we are changing and why. Can someone please give a quick summary? I would also like @andresgalante to review.

If we are changing the code in the PF core repo, should a PR be sent against the PatternFly repo or the website? Looks like this PR is no change the example on the website? @LHinson FYI

@LHinson
Copy link
Member

LHinson commented Jan 11, 2018

I think this is meant to be a change against core (patternfly-patternfly) vs against patternfly-org.

@jgiardino
Copy link
Contributor

@mcarrano We should only need a change to the css files in patternfly core for this and can close this PR.

The way the css is currently written is tightly coupled with how the html is structured. But the react component cannot reproduce this html exactly the way the css selectors expect it to be. We don't need to change the current html. We only need to add an additional class to the existing styles for the secondary navigation so that we can apply those styles by adding this class to the react component.

I added comments to this PR to describe the changes that we need: patternfly/patternfly#906

@dabeng
Copy link
Contributor Author

dabeng commented Jan 12, 2018

Hi @mcarrano, the quick summary is shown in the following picture. Could you tell me which is your favorite "secondary tabs" ? In other words, which one is more likely to be widely used ?
screen shot 2018-01-12 at 1 31 16 pm

@mcarrano
Copy link
Member

@dabeng Nesting the tabs like this was not what was intended. I could see where this may be useful, but to me it becomes more like a nav bar and I don't think we would want to couple the tabs that way unless a specific application is calling for this design.

@LHinson
Copy link
Member

LHinson commented Jan 15, 2018

I'm going to close this PR as changes should be introduced to core. Thanks!

@LHinson LHinson closed this Jan 15, 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.

secondary tabs are not nested in primary tabs

4 participants