Skip to content

Conversation

@wise-king-sullyman
Copy link
Collaborator

@wise-king-sullyman wise-king-sullyman commented Apr 6, 2022

What: Closes #6860

This PR also adds a prop to apply the noBorderBottom styling as needed by the associated demo, and a test to verify that prop works as expected.

Additional issues:

Convenience link directly to the added demo: https://patternfly-react-pr-7183.surge.sh/components/tabs/react-demos/tables-and-tabs-auto-width-tabs/

@patternfly-build
Copy link
Collaborator

patternfly-build commented Apr 6, 2022

Copy link
Member

@mcarrano mcarrano left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks @wise-king-sullyman !

Copy link
Contributor

@kmcfaul kmcfaul left a comment

Choose a reason for hiding this comment

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

LGTM overall just a minor question about prop name which I'd be happy with either way.

/** Enables vertical tab styling */
isVertical?: boolean;
/** Enables no border bottom tab styling */
hasNoBorderBottom?: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

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

Not a blocker but I wonder whether the prop should be called hasBorderBottom with a false value instead of hasNoBorderBottom with a true value? I know the css prop is called noBorderBottom but I feel like I don't see "no{X}" props as much.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah I also didn't like that prop name here for that reason, but having it not match the class name also didn't feel ideal. If you prefer that route though I'm 100% on board.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think making it feel more intuitive to the consumer might be better than making it match the core class name. Im in support of avoiding the double negative.

@nicolethoen
Copy link
Contributor

Could you add a sentence or two in the docs to clarify what this demo is highlighting?

@nicolethoen nicolethoen merged commit f593f9a into patternfly:main Apr 13, 2022
@wise-king-sullyman wise-king-sullyman deleted the tabs-tables-auto-width-demo branch April 13, 2022 20:25
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.

Tables and tabs with auto width tabs

7 participants