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(StyledSystem): Adds styled-system components to map to Core glob… #836

Closed
wants to merge 2 commits into from

Conversation

jschuler
Copy link
Collaborator

…al CSS

Closes:
#753

These components give consumers another avenue to style their pages and components so that they are consistent with the PF-Core theme, e.g. for media breakpoints, paddings, margins, colors, fonts, etc.

@jschuler jschuler added the PF4 React issues for the PF4 core effort label Oct 25, 2018
@patternfly-build
Copy link
Contributor

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

@coveralls
Copy link

coveralls commented Oct 25, 2018

Pull Request Test Coverage Report for Build 2834

  • 18 of 18 (100.0%) changed or added relevant lines in 4 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.06%) to 81.222%

Totals Coverage Status
Change from base Build 2821: 0.06%
Covered Lines: 3441
Relevant Lines: 3994

💛 - Coveralls

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.

Looks good.

@jschuler
Copy link
Collaborator Author

Added do not merge label since i saw a few things that i want to cleanup first

@@ -0,0 +1,62 @@
import { SFC, HTMLProps, ReactNode } from 'react';

export interface StyledBoxProps {
Copy link
Contributor

Choose a reason for hiding this comment

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

These should be available from styled-system @types/styled-system I am still not sure the best way to handle that though. That can either be listed as a peerDependency, and optional, or a straight dependency. @priley86 might have a better idea for that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Okay cool, I can extend my interface with those typings. (just fixing a small issue with them here as well DefinitelyTyped/DefinitelyTyped#30170).
I am adding it as an optional dependency for now, sound good?

const StyledBox = ({ className, children, component, ...props }) => {
const StyledBoxBaseWithComponent = StyledBoxBase.withComponent(component);
return (
<ThemeProvider theme={StyledTheme}>
Copy link
Contributor

Choose a reason for hiding this comment

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

Wonder if we should take the time to add a PatternFlyThemeProvider that contains this. That would allow people to overwrite the theme anywhere in the tree. Also, it would take some of the overhead out of this.

// use values directly so that the user can concatenate multiple shadows
// e.g. boxShadow={`${StyledConstants.sm_right}, ${StyledConstants.sm_bottom}`}
shadows: {
sm: global_BoxShadow_sm.value,
Copy link
Contributor

Choose a reason for hiding this comment

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

Does using the global_BoxShadow_sm.var work for all of these? That would allow people to choose to overwrite using JS or CSS Custom Properties

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, have switched what I could do .var instead. Addressed in #891

@priley86
Copy link
Member

priley86 commented Oct 26, 2018

this will be a huge help.... @jschuler to the rescue
image

@priley86
Copy link
Member

I am fine w/ iterating on these... these constants will likely change over time, but this gives us a very useful baseline.

@priley86
Copy link
Member

priley86 commented Nov 5, 2018

my vote would be to get this PR in a.s.a.p. and iterate in future issues/PRs. I think it will provide a lot of value to many teams now (and some of the PRs coming into PF-R) and would allow less refactor later on...thoughts?

@jschuler
Copy link
Collaborator Author

jschuler commented Nov 7, 2018

Creating a @patternfly/react-styled-system package today then going to add this component to it

@priley86
Copy link
Member

priley86 commented Nov 7, 2018

this is great news. nice job! I think this interface will potentially help a lot of React components be more versatile and meet PF design specification. I think in the future one could even break out the underlying emotion objects constructed into a base package consumable by other frameworks/systems, but that is definitely less pressing...

@jschuler
Copy link
Collaborator Author

jschuler commented Nov 8, 2018

Closing in favor of #891

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Do Not Merge PF4 React issues for the PF4 core effort
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants