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

feat(Breadcrumb): Convert Breadcrumb jsx file to tsx by jessiehuff · Pull Request #1723 · patternfly/patternfly-react · GitHub

/ patternfly-react Public

feat(Breadcrumb): Convert Breadcrumb jsx file to tsx - #1723

Merged
tlabaj merged 14 commits into
patternfly:masterfrom
jessiehuff:feat/breadcrumbTS
May 20, 2019
ConversationCommits (14)ChecksFiles changed
Merged

feat(Breadcrumb): Convert Breadcrumb jsx file to tsx#1723
tlabaj merged 14 commits into
patternfly:masterfrom
jessiehuff:feat/breadcrumbTS

Conversation

jessiehuff commented Apr 8, 2019 •
edited by dlabaj
Loading

Copy link
Copy Markdown
Contributor

Fixes issue #1717 by updating breadcrumb to typescript.

dlabaj requested review from dlabaj, redallen and tlabaj April 8, 2019 18:15
dlabaj requested changes Apr 8, 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

A few comments. Also once this gets working I would added these components to our typescript app under react-integration/demo-app-ts

Comment thread package.json Outdated
Comment thread packages/patternfly-4/react-core/src/components/Breadcrumb-2/Breadcrumb.md Outdated
Comment thread packages/patternfly-4/react-core/src/components/Breadcrumb/Breadcrumb.test.tsx Outdated
Comment thread packages/patternfly-4/react-core/src/components/Breadcrumb/Breadcrumb.tsx Outdated
Comment thread packages/patternfly-4/react-core/src/components/Breadcrumb/BreadcrumbHeading.tsx Outdated
Comment thread packages/patternfly-4/react-core/src/components/Breadcrumb/BreadcrumbItem.tsx Outdated
Comment thread packages/patternfly-4/react-core/src/components/Breadcrumb/index.ts Outdated
dlabaj self-assigned this Apr 8, 2019
dlabaj added the PF4 label Apr 8, 2019

codecov-io commented Apr 8, 2019 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

Merging #1723 into master will increase coverage by 0.01%.
The diff coverage is 91.11%.

@@            Coverage Diff             @@
##           master    #1723      +/-   ##
==========================================
+ Coverage   81.86%   81.88%   +0.01%     
==========================================
  Files         630      631       +1     
  Lines        7214     7241      +27     
  Branches      291      312      +21     
==========================================
+ Hits         5906     5929      +23     
  Misses       1164     1164              
- Partials      144      148       +4
Flag Coverage Δ
#patternfly3 84.88% <ø> (ø) ⬆️
#patternfly4 78.03% <91.11%> (+0.05%) ⬆️
#patternflymisc 95.68% <ø> (ø) ⬆️
Impacted Files Coverage Δ
...eact-core/src/components/Breadcrumb/Breadcrumb.tsx 100% <100%> (ø)
...ly-4/react-core/src/components/Breadcrumb/index.ts 100% <100%> (ø)
...re/src/components/Breadcrumb/BreadcrumbHeading.tsx 85.71% <85.71%> (ø)
...-core/src/components/Breadcrumb/BreadcrumbItem.tsx 89.47% <89.47%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2b9d252...84e0227. Read the comment docs.

patternfly-build commented Apr 9, 2019

Copy link
Copy Markdown
Collaborator

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

redallen previously approved these changes Apr 9, 2019
View reviewed changes

redallen 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

Good start.

dlabaj previously approved these changes Apr 10, 2019
View reviewed changes
kmcfaul previously approved these changes Apr 15, 2019
View reviewed changes
tlabaj suggested changes May 2, 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

A few questions and updates.

Comment thread packages/patternfly-4/react-core/src/components/Breadcrumb/Breadcrumb.tsx
Comment thread packages/patternfly-4/react-core/src/components/Breadcrumb/BreadcrumbHeading.tsx
Comment thread packages/patternfly-4/react-core/src/components/Breadcrumb/BreadcrumbItem.tsx
Comment thread ...ges/patternfly-4/react-core/src/components/Breadcrumb/__snapshots__/Breadcrumb.test.tsx.snap
Comment thread package.json
Comment thread yarn.lock Outdated
jessiehuff dismissed stale reviews from kmcfaul, dlabaj, and redallen via a0a49b3 May 6, 2019 18:03
jessiehuff force-pushed the feat/breadcrumbTS branch from a2aa6e3 to 788ed89 Compare May 6, 2019 20:11

patternfly-build commented May 6, 2019

Copy link
Copy Markdown
Collaborator

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

jessiehuff force-pushed the feat/breadcrumbTS branch 2 times, most recently from a2aa6e3 to 473ed94 Compare May 6, 2019 21:02

nicolethoen commented May 8, 2019

Copy link
Copy Markdown
Contributor

@jessiehuff can you also add integration tests for this component as outlined on this README?

tlabaj suggested changes May 10, 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

We need to figure out why the test case is rendering the as any rather than 'a'

Comment thread ...ges/patternfly-4/react-core/src/components/Breadcrumb/__snapshots__/Breadcrumb.test.tsx.snap
redallen suggested changes May 15, 2019
View reviewed changes
Comment thread packages/patternfly-4/react-core/src/components/Breadcrumb/Breadcrumb.tsx Outdated
Comment thread packages/patternfly-4/react-core/src/components/Breadcrumb/Breadcrumb.tsx Outdated
Comment thread packages/patternfly-4/react-core/src/components/Breadcrumb/BreadcrumbHeading.tsx Outdated
Comment thread packages/patternfly-4/react-core/src/components/Breadcrumb/BreadcrumbHeading.tsx Outdated
Comment thread packages/patternfly-4/react-core/src/components/Breadcrumb/BreadcrumbItem.tsx Outdated
Comment thread packages/patternfly-4/react-integration/README.md Outdated
jessiehuff force-pushed the feat/breadcrumbTS branch from 7ca295a to 76ac93e Compare May 15, 2019 19:18
tlabaj suggested changes May 16, 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

A few more comments.

Comment thread packages/patternfly-4/react-core/src/components/Breadcrumb/Breadcrumb.tsx Outdated
Comment thread packages/patternfly-4/react-core/src/components/Breadcrumb/BreadcrumbHeading.tsx Outdated
Comment thread packages/patternfly-4/react-core/src/components/Breadcrumb/BreadcrumbItem.tsx Outdated
tlabaj reviewed May 16, 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

A few more comments.

Comment thread packages/patternfly-4/react-core/src/components/Breadcrumb/BreadcrumbHeading.tsx Outdated
tlabaj suggested changes May 17, 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 like this was updated. I still see some things that are not quite right.

Comment thread packages/patternfly-4/react-core/src/components/Breadcrumb/BreadcrumbHeading.tsx Outdated
Comment thread packages/patternfly-4/react-core/src/components/Breadcrumb/Breadcrumb.tsx Outdated
tlabaj suggested changes May 17, 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

The component?: React.ReactType; Still is not right.

redallen previously approved these changes May 20, 2019
View reviewed changes

redallen 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

Thanks for this!

Comment thread packages/patternfly-4/react-core/src/components/Page/__snapshots__/Page.test.js.snap
dlabaj requested changes May 20, 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

One update to integration test other then that looks good.

Comment thread packages/patternfly-4/react-integration/cypress/integration/breadcrumb.spec.ts Outdated
tlabaj approved these changes May 20, 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

tlabaj merged commit 9151262 into patternfly:master May 20, 2019
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 approved these changes
kmcfaul kmcfaul left review comments
+2 more reviewers
tlabaj tlabaj approved these changes
redallen redallen left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

dlabaj

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

Back | FazBrowse Home | New Git URL