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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 14 commits into from
May 20, 2019
Merged

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

merged 14 commits into from
May 20, 2019

Conversation

jessiehuff
Copy link
Contributor

@jessiehuff jessiehuff commented Apr 8, 2019 •
edited by dlabaj
Loading

Fixes issue #1717 by updating breadcrumb to typescript.

@dlabaj dlabaj requested review from dlabaj, tlabaj and redallen April 8, 2019 18:15
dlabaj
dlabaj requested changes Apr 8, 2019
View reviewed changes
Copy link
Contributor

@dlabaj dlabaj left a comment

Choose a reason for hiding this comment

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

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

package.json Outdated Show resolved Hide resolved
packages/patternfly-4/react-core/src/components/Breadcrumb-2/Breadcrumb.md Outdated Show resolved Hide resolved
packages/patternfly-4/react-core/src/components/Breadcrumb/Breadcrumb.test.tsx Outdated Show resolved Hide resolved
packages/patternfly-4/react-core/src/components/Breadcrumb/Breadcrumb.tsx Outdated Show resolved Hide resolved
packages/patternfly-4/react-core/src/components/Breadcrumb/BreadcrumbHeading.tsx Outdated Show resolved Hide resolved
packages/patternfly-4/react-core/src/components/Breadcrumb/BreadcrumbItem.tsx Outdated Show resolved Hide resolved
packages/patternfly-4/react-core/src/components/Breadcrumb/index.ts Outdated Show resolved Hide resolved
@dlabaj dlabaj self-assigned this Apr 8, 2019
@dlabaj dlabaj added the PF4 label Apr 8, 2019
@codecov-io
Copy link

codecov-io commented Apr 8, 2019 •
edited
Loading

Codecov Report

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

Impacted file tree graph

@@            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
Copy link
Contributor

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

redallen
redallen previously approved these changes Apr 9, 2019
View reviewed changes
Copy link
Contributor

@redallen redallen left a comment

Choose a reason for hiding this comment

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

Good start.

dlabaj
dlabaj previously approved these changes Apr 10, 2019
View reviewed changes
kmcfaul
kmcfaul previously approved these changes Apr 15, 2019
View reviewed changes
tlabaj
tlabaj requested changes May 2, 2019
View reviewed changes
Copy link
Contributor

@tlabaj tlabaj left a comment

Choose a reason for hiding this comment

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

A few questions and updates.

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

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

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

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

tlabaj
tlabaj requested changes May 10, 2019
View reviewed changes
Copy link
Contributor

@tlabaj tlabaj left a comment

Choose a reason for hiding this comment

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

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

...ges/patternfly-4/react-core/src/components/Breadcrumb/__snapshots__/Breadcrumb.test.tsx.snap Show resolved Hide resolved
@jessiehuff jessiehuff force-pushed the feat/breadcrumbTS branch from 7ca295a to 76ac93e Compare May 15, 2019 19:18
tlabaj
tlabaj requested changes May 16, 2019
View reviewed changes
Copy link
Contributor

@tlabaj tlabaj left a comment

Choose a reason for hiding this comment

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

A few more comments.

packages/patternfly-4/react-core/src/components/Breadcrumb/Breadcrumb.tsx Outdated Show resolved Hide resolved
packages/patternfly-4/react-core/src/components/Breadcrumb/BreadcrumbHeading.tsx Outdated Show resolved Hide resolved
packages/patternfly-4/react-core/src/components/Breadcrumb/BreadcrumbItem.tsx Outdated Show resolved Hide resolved
tlabaj
tlabaj reviewed May 16, 2019
View reviewed changes
Copy link
Contributor

@tlabaj tlabaj left a comment

Choose a reason for hiding this comment

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

A few more comments.

packages/patternfly-4/react-core/src/components/Breadcrumb/BreadcrumbHeading.tsx Outdated Show resolved Hide resolved
tlabaj
tlabaj requested changes May 17, 2019
View reviewed changes
Copy link
Contributor

@tlabaj tlabaj left a comment

Choose a reason for hiding this comment

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

Looks like this was updated. I still see some things that are not quite right.

packages/patternfly-4/react-core/src/components/Breadcrumb/BreadcrumbHeading.tsx Outdated Show resolved Hide resolved
packages/patternfly-4/react-core/src/components/Breadcrumb/Breadcrumb.tsx Outdated Show resolved Hide resolved
tlabaj
tlabaj requested changes May 17, 2019
View reviewed changes
Copy link
Contributor

@tlabaj tlabaj left a comment

Choose a reason for hiding this comment

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

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

redallen
redallen previously approved these changes May 20, 2019
View reviewed changes
Copy link
Contributor

@redallen redallen left a comment

Choose a reason for hiding this comment

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

Thanks for this!

packages/patternfly-4/react-core/src/components/Page/__snapshots__/Page.test.js.snap Show resolved Hide resolved
dlabaj
dlabaj requested changes May 20, 2019
View reviewed changes
Copy link
Contributor

@dlabaj dlabaj left a comment

Choose a reason for hiding this comment

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

One update to integration test other then that looks good.

packages/patternfly-4/react-integration/cypress/integration/breadcrumb.spec.ts Outdated Show resolved Hide resolved
tlabaj
tlabaj approved these changes May 20, 2019
View reviewed changes
Copy link
Contributor

@tlabaj tlabaj left a comment

Choose a reason for hiding this comment

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

LGTM

@tlabaj tlabaj merged commit 9151262 into patternfly:master May 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dlabaj dlabaj dlabaj approved these changes

@tlabaj tlabaj tlabaj approved these changes

@kmcfaul kmcfaul kmcfaul left review comments

@redallen redallen redallen left review comments

Assignees

@dlabaj dlabaj

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@jessiehuff @codecov-io @patternfly-build @nicolethoen @dlabaj @tlabaj @kmcfaul @redallen