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

PF4 - use context api in page component #2208

Merged
merged 3 commits into from
Jun 19, 2019

Conversation

jschuler
Copy link
Collaborator

Closes: #2188

@patternfly-build
Copy link
Contributor

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

@codecov-io
Copy link

codecov-io commented Jun 11, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@14d1d70). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #2208   +/-   ##
=========================================
  Coverage          ?   80.27%           
=========================================
  Files             ?      658           
  Lines             ?     8252           
  Branches          ?      643           
=========================================
  Hits              ?     6624           
  Misses            ?     1326           
  Partials          ?      302
Flag Coverage Δ
#patternfly3 85.23% <ø> (?)
#patternfly4 75.82% <100%> (?)
Impacted Files Coverage Δ
...atternfly-4/react-core/src/components/Page/Page.js 56% <100%> (ø)
...fly-4/react-core/src/components/Page/PageHeader.js 90% <100%> (ø)
...ly-4/react-core/src/components/Page/PageSidebar.js 100% <100%> (ø)

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 14d1d70...30bea49. Read the comment docs.

)}
</div>
)}
{/* Hide for now until we have the context selector component */}
Copy link
Contributor

Choose a reason for hiding this comment

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

We have . context Selector. Why are we hiding this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This has been here since before the context selector got created. Perhaps we should update the page component now to support it? Should be in a separate PR though. @rachael-phillips

Copy link
Contributor

@tlabaj tlabaj Jun 17, 2019

Choose a reason for hiding this comment

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

agreed. We should do tis in a separate PR. @rachael-phillips I opened a separate issue (#2285) to track that work.

redallen
redallen previously approved these changes Jun 14, 2019
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.

Fix merge conflicts and I'll reapprove!

@tlabaj tlabaj closed this Jun 17, 2019
@tlabaj tlabaj reopened this Jun 17, 2019
@patternfly-build
Copy link
Contributor

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

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

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.

LGTM

@dlabaj dlabaj merged commit 1251326 into patternfly:master Jun 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Page component isManagedSidebar does not work if wrapper components are used for the header and sidebar props
6 participants