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

feat(tab): support JSX elements in Tab title #2681

Merged
merged 2 commits into from Sep 6, 2019

Conversation

@boaz0
Copy link
Member

boaz0 commented Aug 11, 2019

What:

closes #2623

In general Tab supported JSX elements the problem is that the title props type was set to string. In addition, Tab didn't use the title prop at all, which was confusing, because it was passed as a child in the Tabs component. So what I did was removing title from props.

//cc @xeviknal

@patternfly-build

This comment has been minimized.

Copy link
Contributor

patternfly-build commented Aug 11, 2019

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

@tlabaj tlabaj self-requested a review Aug 12, 2019
@tlabaj tlabaj self-assigned this Aug 12, 2019
@xeviknal

This comment has been minimized.

Copy link

xeviknal commented Aug 19, 2019

@tlabaj looks great to me!

@karelhala

This comment has been minimized.

Copy link
Contributor

karelhala commented Aug 27, 2019

I didn't noticed that this PR was opened and opened another one #2766. Sorry for that @boaz0, care to rebase to newest version so we can merge this one?

@boaz0

This comment has been minimized.

Copy link
Member Author

boaz0 commented Aug 27, 2019

@karelhala sure and thank you for noticing 😃 🙏

Copy link
Contributor

karelhala left a comment

Looking good!

Copy link
Contributor

dlabaj left a comment

Looks good just a few comments. Can you also update the Tabs integration tests to test for JSX. You would have to update one of the tabs in the demo app

<Tab id="demoTab2" eventKey={1} title="Tab item 2" tabContentId="demoTab2Section" tabContentRef={this.contentRef2} />
and then update the test to check for the JSX element
expect(demoSection.text()).to.equal(`Tab ${currentItem} section`);

Copy link
Member

mcarrano left a comment

@boaz0 I am fine with updating the tab to allow for elements other than text. My only request is that you change the first example that shows the icon in the tab to something more generic (a status perhaps?). The icon being used is for an external link which is not something I would recommend using a Tab for. This might be confusing or suggest a design pattern that we don't recommend in PatternFly. Thanks.

@boaz0

This comment has been minimized.

Copy link
Member Author

boaz0 commented Sep 2, 2019

@mcarrano no problem!

@boaz0 boaz0 force-pushed the boaz0:closes_2623 branch from 195cc47 to 7420653 Sep 2, 2019
@boaz0

This comment has been minimized.

Copy link
Member Author

boaz0 commented Sep 2, 2019

@dlabaj - addressed your comments :) thanks for the review btw.

Signed-off-by: Boaz Shuster <boaz.shuster.github@gmail.com>
@boaz0 boaz0 force-pushed the boaz0:closes_2623 branch from 7420653 to c0eabf7 Sep 2, 2019
Copy link
Contributor

karelhala left a comment

Looking good! Thank you for adding the omit and correct type.

@tlabaj
tlabaj approved these changes Sep 4, 2019
Copy link
Contributor

tlabaj left a comment

Looks great. Thanks for the updates

Copy link
Contributor

tlabaj left a comment

typo in MD file is causing test to fail.

@@ -8,12 +8,14 @@ typescript: true
## Simple tabs

import { Tabs, Tab, TabsVariant, TabContent } from '@patternfly/react-core';
import { AddressBookIcon } from '@patternfly/react-icons`';

This comment has been minimized.

Copy link
@tlabaj

tlabaj Sep 4, 2019

Contributor

looks like a typo here. This is causing the test to fail.

@tlabaj tlabaj dismissed stale reviews from dlabaj and karelhala via ab067c2 Sep 6, 2019
Copy link
Member

mcarrano left a comment

Looks good. Thanks for changing the example @boaz0

@tlabaj
tlabaj approved these changes Sep 6, 2019
Copy link
Contributor

tlabaj left a comment

LGTM

@redallen redallen merged commit e6d04fd into patternfly:master Sep 6, 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 6, 2019

Your changes have been released in:

  • @patternfly/react-charts@4.9.6
  • @patternfly/react-core@3.101.0
  • @patternfly/react-docs@4.12.1
  • @patternfly/react-inline-edit-extension@2.11.27
  • demo-app-ts@2.24.0
  • @patternfly/react-integration@2.24.0
  • @patternfly/react-styled-system@3.6.31
  • @patternfly/react-styles@3.5.20
  • @patternfly/react-table@2.20.7
  • @patternfly/react-tokens@2.6.24
  • @patternfly/react-topology@2.8.26
  • @patternfly/react-virtualized-extension@1.2.15
  • @patternfly/react-icons@3.13.1

Thanks for your contribution! 🎉

@boaz0

This comment has been minimized.

Copy link
Member Author

boaz0 commented Sep 7, 2019

Thanks @tlabaj for fixing the typo!

@boaz0 boaz0 deleted the boaz0:closes_2623 branch Sep 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.