React < 16.3 support (WIP) #1113
Conversation
There was a problem hiding this comment.
Thank you! This is super cool! ❤️
One sad note: could you please make a PR from the next branch? And I'd probably wait until the Enzyme issue is resolved, we can't merge it with broken tests. In the worst case, I'll change peer deps in 8.0.0 and we'll merge this is a path release later. 🙏
| @@ -159,7 +158,7 @@ | |||
| "test:browser:customised": "node test/browser.js examples/customised/styleguide/index.html", | |||
| "test:browser:sections": "node test/browser.js examples/sections/styleguide/index.html", | |||
| "precommit": "lint-staged", | |||
| "format": "prettier --write '**/*.{js,md}'", | |||
| "format": "prettier --write \"**/*.{js,md}\"", | |||
There was a problem hiding this comment.
Yes, I had problems in Windows, it relates with this issue:
prettier/prettier#4086 (comment)
And here explains that
https://prettier.io/docs/en/cli.html
Don't forget the quotes around the globs! The quotes make sure that Prettier expands the globs rather than your shell, for cross-platform usage.
Maybe it should be other PR
There was a problem hiding this comment.
🤦♂️
We should change it here, otherwise the script will overwrite the command eventually:
https://github.com/sapegin/mrm-tasks/blob/master/packages/mrm-task-prettier/index.js#L82
There was a problem hiding this comment.
Amazing project @sapegin!
|
|
||
| Provider.propTypes = { | ||
| children: PropTypes.any, | ||
| codeRevision: PropTypes.number.isRequired, |
There was a problem hiding this comment.
You're not using some of these props.
There was a problem hiding this comment.
I have copied from this childContextTypes
https://github.com/styleguidist/react-styleguidist/blob/master/src/rsg-components/StyleGuide/StyleGuide.js#L45-L50
because they are used in several components
|
|
||
| export const Context = React.createContext(); | ||
|
|
||
| export function Consumer(WrappedComponent) { |
There was a problem hiding this comment.
I'd call it ConfigConsumer to make more clear what it does.
| @@ -0,0 +1,39 @@ | |||
| import React, { Component } from 'react'; | |||
There was a problem hiding this comment.
I'd move it to rsg-components like any other component, and rename to ConfigProvider.
|
|
||
| export function Provider(props) { | ||
| const { children, ...rest } = props; | ||
| const ui = typeof children === 'function' ? children(this.state) : children; |
There was a problem hiding this comment.
It will be removed!
| const Styled = styledWrapper(styles, WrappedComponent, componentName); | ||
|
|
||
| return class extends Component { | ||
| static displayName = `Consumer(${componentName})`; |
There was a problem hiding this comment.
We probably shouldn't show the same name as for context consumer.
There was a problem hiding this comment.
Well, it was created to give a name, I wanted to make something similar like Styled(Component), but yes, I will remove it
|
|
||
| return class extends Component { | ||
| static displayName = `Consumer(${componentName})`; | ||
| state = {}; |
There was a problem hiding this comment.
Looks like you don't need state here.
There was a problem hiding this comment.
Yes, I will change it
| @@ -1,26 +1,22 @@ | |||
| import React, { Component } from 'react'; | |||
| import PropTypes from 'prop-types'; | |||
| import createStyleSheet from '../../styles/createStyleSheet'; | |||
| import { Context } from '../../provider'; | |||
There was a problem hiding this comment.
I couldn't use it, because Consumer gives to component all context as props (codeRevision, config, slots and displayMode), and Styled only needs the object config.
When I use Consumer, in Styled appears an error, because all context's properties are used here
https://github.com/styleguidist/react-styleguidist/blob/master/src/rsg-components/Styled/Styled.js#L15
So for that reason, I created a custom Consumer that gives only config property
|
No problem, I will create another PR taking |
Issue #1094