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, Nav): add dark theme modifier flag #2856

Merged
merged 4 commits into from Sep 6, 2019

Conversation

@kmcfaul
Copy link
Contributor

kmcfaul commented Sep 5, 2019

What: adds dark theme opt-in option to page sidebar and nav components. Example added to PageLayout demos

Refer to issue: #2782

@patternfly-build

This comment has been minimized.

Copy link
Contributor

patternfly-build commented Sep 5, 2019

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

@christiemolloy

This comment has been minimized.

Copy link
Member

christiemolloy commented Sep 5, 2019

@kmcfaul In Core @mcoker updated all of the page demos to use the dark nav, the nav component to use the dark nav (except for one example with the white), and the page component to use the dark nav. If this issue is pressing I think it would be ok to merge it and then follow up with an issue to update all of those components.

@jschuler

This comment has been minimized.

Copy link
Collaborator

jschuler commented Sep 5, 2019

If you change the Nav examples to use theme="dark" in the Nav.md file can you also add a frontmatter prop for optIn with a description?
e.g.

---
title: 'Nav'
cssPrefix: 'pf-c-nav'
typescript: true
propComponents: ['Nav', 'NavList', 'NavGroup', 'NavItem', 'NavItemSeparator', 'NavExpandable']
optIn: 'My description here'
---

@rachael-phillips is there a specific description we should use?

@rachael-phillips

This comment has been minimized.

Copy link
Contributor

rachael-phillips commented Sep 5, 2019

If you change the Nav examples to use theme="dark" in the Nav.md file can you also add a frontmatter prop for optIn with a description?
e.g.

---
title: 'Nav'
cssPrefix: 'pf-c-nav'
typescript: true
propComponents: ['Nav', 'NavList', 'NavGroup', 'NavItem', 'NavItemSeparator', 'NavExpandable']
optIn: 'My description here'
---

@rachael-phillips is there a specific description we should use?

@Christie @joachim does this description make sense to you "Opt-in and update to the latest design by using this prop"

@kmcfaul

This comment has been minimized.

Copy link
Contributor Author

kmcfaul commented Sep 5, 2019

Working on updating the demos and adding the legacy/light demo, will update asap

@jschuler

This comment has been minimized.

Copy link
Collaborator

jschuler commented Sep 5, 2019

@rachael-phillips almost, have to also call out what the user needs to set I think.

In a future breaking-change release, the nav will default to the dark theme. You can opt-in and update to use the nav dark theme now by setting the theme prop to 'dark'.

@rachael-phillips

This comment has been minimized.

Copy link
Contributor

rachael-phillips commented Sep 5, 2019

@rachael-phillips almost, have to also call out what the user needs to set I think.

In a future breaking-change release, the nav will default to the dark theme. You can opt-in and update to use the nav dark theme now by setting the theme prop to 'dark'.

@jschuler that looks fantastic to me. You're a genius - let's go with that. @kmcfaul any concerns or additions to that approach?

@kmcfaul

This comment has been minimized.

Copy link
Contributor Author

kmcfaul commented Sep 5, 2019

Looks good to me

@kmcfaul

This comment has been minimized.

Copy link
Contributor Author

kmcfaul commented Sep 5, 2019

Added in legacy/light example and swapped other page and nav examples to dark theme. Added in the optIn frontmatter prop to Nav (should this go on Page somewhere too?). Fixed the missing type from PageSidebar's theme prop.

@kmcfaul

This comment has been minimized.

Copy link
Contributor Author

kmcfaul commented Sep 5, 2019

@redallen Any thoughts on the docs build error? I tried nuking my cache and rerunning just that step locally but it runs fine

edit: nevermind got it

Copy link
Contributor

mcoker left a comment

Looks great!! Thanks @kmcfaul!! ⭐️

@lboehling

This comment has been minimized.

Copy link

lboehling commented Sep 6, 2019

This looks great! Only one thing to note, the divider line in the expanded section should be pf-black-850 (#212427) and the divider line outside of expanded section on main nav bkg should be pf-black-700 (#4F5255).

@kmcfaul

This comment has been minimized.

Copy link
Contributor Author

kmcfaul commented Sep 6, 2019

the divider line in the expanded section should be pf-black-850 (#212427) and the divider line outside of expanded section on main nav bkg should be pf-black-700 (#4F5255).

@mcoker Are these overrides I need put in the example css as well?

@mcoker

This comment has been minimized.

Copy link
Contributor

mcoker commented Sep 6, 2019

@kmcfaul nope, that's a bug I need to fix in core. We're using the <Divider> component in the core workspace, and the react workspace uses <NavItemSeparator>, which is still valid. I'll push a fix ASAP.

@kmcfaul kmcfaul dismissed stale reviews from jschuler and mcoker via 0f9dc9a Sep 6, 2019
@kmcfaul kmcfaul force-pushed the kmcfaul:dark-opt-in branch from 8835f53 to 0f9dc9a Sep 6, 2019
@tlabaj tlabaj added css review and removed css approved labels Sep 6, 2019
@tlabaj
tlabaj approved these changes Sep 6, 2019
Copy link
Contributor

tlabaj left a comment

LGTM

Copy link
Member

christiemolloy left a comment

thanks for making those changes

@jschuler jschuler merged commit 9c5f336 into patternfly:master Sep 6, 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
Copy link
Member

mcarrano left a comment

Looks good to me

@patternfly-build

This comment has been minimized.

Copy link
Contributor

patternfly-build commented Sep 6, 2019

Your changes have been released in:

  • @patternfly/react-core@3.102.0
  • @patternfly/react-docs@4.12.2
  • @patternfly/react-inline-edit-extension@2.11.28
  • demo-app-ts@2.24.1
  • @patternfly/react-table@2.20.8
  • @patternfly/react-topology@2.8.27
  • @patternfly/react-virtualized-extension@1.2.16

Thanks for your contribution! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants
You can’t perform that action at this time.