FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

feat(tab): support JSX elements in Tab title by boaz0 · Pull Request #2681 · patternfly/patternfly-react · GitHub

/ patternfly-react Public

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

Merged
redallen merged 2 commits into
patternfly:masterfrom
boaz0:closes_2623
Sep 6, 2019
ConversationCommits (2)ChecksFiles changed
Merged

feat(tab): support JSX elements in Tab title#2681
redallen merged 2 commits into
patternfly:masterfrom
boaz0:closes_2623

Conversation

boaz0 commented Aug 11, 2019

Copy link
Copy Markdown
Member

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 commented Aug 11, 2019

Copy link
Copy Markdown
Collaborator

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

tlabaj self-requested a review August 12, 2019 20:00
tlabaj self-assigned this Aug 12, 2019

xeviknal commented Aug 19, 2019

Copy link
Copy Markdown

@tlabaj looks great to me!

karelhala mentioned this pull request Aug 27, 2019
Closed

karelhala commented Aug 27, 2019

Copy link
Copy Markdown
Contributor

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 commented Aug 27, 2019 •
edited
Loading

Copy link
Copy Markdown
Member Author

@karelhala sure and thank you for noticing 😃 🙏

karelhala mentioned this pull request Aug 27, 2019
Closed
aljesusg mentioned this pull request Aug 27, 2019
Closed
karelhala previously approved these changes Aug 28, 2019
View reviewed changes

karelhala left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Looking good!

dlabaj reviewed Aug 29, 2019
View reviewed changes

dlabaj left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

patternfly-react/packages/patternfly-4/react-integration/demo-app-ts/src/components/demos/TabsDemo/TabsDemo.tsx

Line 36 in d6ee538

<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

patternfly-react/packages/patternfly-4/react-integration/cypress/integration/tabdemo.spec.ts

Line 15 in d6ee538

expect(demoSection.text()).to.equal(`Tab ${currentItem} section`);

Comment thread packages/patternfly-4/react-core/src/components/Tabs/Tab.tsx Outdated
mcarrano suggested changes Aug 30, 2019
View reviewed changes

mcarrano left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@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 commented Sep 2, 2019

Copy link
Copy Markdown
Member Author

@mcarrano no problem!

boaz0 commented Sep 2, 2019

Copy link
Copy Markdown
Member Author

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

feat(tab): support JSX elements in Tab title
c0eabf7 
Signed-off-by: Boaz Shuster <boaz.shuster.github@gmail.com>
karelhala previously approved these changes Sep 3, 2019
View reviewed changes

karelhala left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

dlabaj previously approved these changes Sep 4, 2019
View reviewed changes
tlabaj approved these changes Sep 4, 2019
View reviewed changes

tlabaj left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Looks great. Thanks for the updates

tlabaj suggested changes Sep 4, 2019
View reviewed changes

tlabaj left a comment •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

typo in MD file is causing test to fail.

Comment thread packages/patternfly-4/react-core/src/components/Tabs/Tabs.md Outdated
## Simple tabs

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

tlabaj Sep 4, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

tlabaj dismissed stale reviews from dlabaj and karelhala via ab067c2 September 6, 2019 17:10
mcarrano approved these changes Sep 6, 2019
View reviewed changes

mcarrano left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Looks good. Thanks for changing the example @boaz0

tlabaj approved these changes Sep 6, 2019
View reviewed changes

tlabaj left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM

redallen merged commit e6d04fd into patternfly:master Sep 6, 2019

patternfly-build commented Sep 6, 2019

Copy link
Copy Markdown
Collaborator

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 commented Sep 7, 2019

Copy link
Copy Markdown
Member Author

Thanks @tlabaj for fixing the typo!

boaz0 deleted the closes_2623 branch September 7, 2019 22:56
Venefilyn mentioned this pull request Oct 1, 2019
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

dlabaj dlabaj left review comments
+4 more reviewers
mcarrano mcarrano approved these changes
tlabaj tlabaj approved these changes
redallen redallen approved these changes
karelhala karelhala left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

tlabaj

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Feature: Support JSX.Element on Tab title property

8 participants

Back | FazBrowse Home | New Git URL