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

fix(HorizontalNavMenuItem): Allow HorizontalNavMenuItem's title to be a node #2825

Merged

Conversation

@martinpovolny
Copy link
Contributor

martinpovolny commented Sep 4, 2019

Allow HorizontalNavMenuItem's title to be a node rather than just a string.

Needed by: ManageIQ/react-ui-components#115 and ManageIQ/manageiq-ui-classic#5997

Ping @karelhala

@@ -35,7 +35,7 @@ const HorizontalNavMenuItem = props => {
HorizontalNavMenuItem.propTypes = {
children: PropTypes.node,
onItemClick: PropTypes.object,
title: PropTypes.string,
title: PropTypes.node,

This comment has been minimized.

Copy link
@jeff-phillips-18

jeff-phillips-18 Sep 4, 2019

Member

While node works for both, we typically have used:

Suggested change
title: PropTypes.node,
title: PropTypes.oneOfType([PropTypes.string, PropTypes.node]),

to be more explicit

This comment has been minimized.

Copy link
@karelhala

karelhala Sep 4, 2019

Contributor

Not sure that it's required node can be anything that can be rendered https://reactjs.org/docs/typechecking-with-proptypes.html#proptypes

This comment has been minimized.

Copy link
@jeff-phillips-18

jeff-phillips-18 Sep 4, 2019

Member

Understood, just looking for consistency and explicitly noting in the docs.

This comment has been minimized.

Copy link
@martinpovolny

martinpovolny Sep 4, 2019

Author Contributor

Guys, I do not care ;-)

I am seeing both variants in places. Tell me if I need to change it to get the PR merged or not.

Yes - change / No - leave it.

That's what I care about :-D Because w/o that I cannot finish the PRs above.

This comment has been minimized.

Copy link
@martinpovolny

martinpovolny Sep 4, 2019

Author Contributor

Thx!

@jeff-phillips-18

This comment has been minimized.

Copy link
Member

jeff-phillips-18 commented Sep 6, 2019

@martinpovolny Could you rebase your branch to fix the build error?

@martinpovolny martinpovolny dismissed stale reviews from skateman, jeff-phillips-18, and karelhala via 82bba76 Sep 9, 2019
@martinpovolny martinpovolny force-pushed the martinpovolny:hor_nav_item_title_object branch from 270089f to 82bba76 Sep 9, 2019
@martinpovolny

This comment has been minimized.

Copy link
Contributor Author

martinpovolny commented Sep 9, 2019

Rebased.

@patternfly-build

This comment has been minimized.

Copy link
Contributor

patternfly-build commented Sep 9, 2019

PatternFly-React preview: https://patternfly-react-pr-2825.surge.sh

@jeff-phillips-18 jeff-phillips-18 merged commit 78adfa4 into patternfly:master Sep 9, 2019
8 checks passed
8 checks passed
ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: build_integration Your tests passed on CircleCI!
Details
ci/circleci: build_pf3_docs Your tests passed on CircleCI!
Details
ci/circleci: build_pf4_docs Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: test_jest_other Your tests passed on CircleCI!
Details
ci/circleci: test_jest_pf4 Your tests passed on CircleCI!
Details
ci/circleci: upload_docs Your tests passed on CircleCI!
Details
@patternfly-build

This comment has been minimized.

Copy link
Contributor

patternfly-build commented Sep 9, 2019

Your changes have been released in:

  • patternfly-react-extensions@2.20.3
  • patternfly-react@2.38.1
  • @patternfly/react-console@1.12.7

Thanks for your contribution! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.