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(Page): add page section main nav type variant #2268

Merged
merged 4 commits into from Jun 27, 2019

Conversation

@kmcfaul
Copy link
Contributor

kmcfaul commented Jun 13, 2019

What: Adding missing page main nav section

Refer to issue: #1742

@patternfly-build

This comment has been minimized.

Copy link
Contributor

patternfly-build commented Jun 13, 2019

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Jun 13, 2019

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #2268   +/-   ##
=========================================
  Coverage          ?   79.89%           
=========================================
  Files             ?      669           
  Lines             ?     8531           
  Branches          ?      734           
=========================================
  Hits              ?     6816           
  Misses            ?     1362           
  Partials          ?      353
Flag Coverage Δ
#patternfly3 85.23% <ø> (?)
#patternfly4 74.93% <100%> (?)
#patternflymisc 95.79% <ø> (?)
Impacted Files Coverage Δ
...ly-4/react-core/src/components/Page/PageSection.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 a5d571e...b926e4f. Read the comment docs.

Copy link
Contributor

redallen left a comment

This is in conflict with #2240 . Let's wait for that to get in (it's huge) and then rebase.

@kmcfaul kmcfaul force-pushed the kmcfaul:fix-page-main-nav branch from 9c68da7 to 3e670a7 Jun 26, 2019
@patternfly-build

This comment has been minimized.

Copy link
Contributor

patternfly-build commented Jun 26, 2019

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

@tlabaj tlabaj added the css review label Jun 27, 2019
@tlabaj tlabaj requested a review from srambach Jun 27, 2019

export interface PageSectionProps extends React.HTMLProps<HTMLDivElement> {
/** Content rendered inside the section */
children?: React.ReactNode;
/** Additional classes added to the section */
className?: string;
/** Section background color variant */
variant?: 'default' | 'light' | 'dark' | 'darker' | PageSectionVariants
variant?: 'default' | 'light' | 'dark' | 'darker' | PageSectionVariants;

This comment has been minimized.

Copy link
@tlabaj

tlabaj Jun 27, 2019

Contributor

Why are we adding the enum to union here? I don't think it is needed.

This comment has been minimized.

Copy link
@kmcfaul

kmcfaul Jun 27, 2019

Author Contributor

This came in from the TS conversion, which I followed for the type prop I added. Should I remove the enums and just leave the unions?

Edit: Talked with @redallen and I removed them. It's not really needed.

@tlabaj tlabaj requested a review from jschuler Jun 27, 2019

export interface PageSectionProps extends React.HTMLProps<HTMLDivElement> {
/** Content rendered inside the section */
children?: React.ReactNode;
/** Additional classes added to the section */
className?: string;
/** Section background color variant */
variant?: 'default' | 'light' | 'dark' | 'darker' | PageSectionVariants
variant?: 'default' | 'light' | 'dark' | 'darker';
/** Section type variant */

This comment has been minimized.

Copy link
@jschuler

jschuler Jun 27, 2019

Collaborator

when should consumers use nav? should there be an example? maybe a little more descriptive comment?

This comment has been minimized.

Copy link
@tlabaj

tlabaj Jun 27, 2019

Contributor

Agreed. There is no example in core. @srambach will be opening up an additional issue to add one.

@srambach

This comment has been minimized.

Copy link
Member

srambach commented Jun 27, 2019

Results look good to me 👍

@tlabaj
tlabaj approved these changes Jun 27, 2019
Copy link
Contributor

tlabaj left a comment

LGTM

@srambach srambach added css approved and removed css review labels Jun 27, 2019
@redallen redallen merged commit 0c0d4a0 into patternfly:master Jun 27, 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
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.