Update cf-component-label to use Fela #103
Conversation
|
As a reminder Do Not Merge till we start merging Fela components. |
|
please rebase |
manatarms
force-pushed
the
fela-component-label
branch
from
99e412b
to
b5f69fa
Compare
|
@tajo This is good to go. You can merge this and publish into cf-ux when you're ready. |
|
If @tajo approves lets merge and have him release. I'm hesitant to release since I haven't been following the issues as closely recently. |
|
Question: Are these styles using our private styles? |
| import Label from 'cf-component-label'; | ||
| import felaTestContext from '../../../styleguide/felaTestContext'; | ||
| import { applyTheme } from 'cf-style-container'; | ||
| import { Label as LabelUnstyled, LabelTheme } from 'cf-component-label'; |
There was a problem hiding this comment.
please, use relative paths... this will break once this is merged: #101
| const styles = props => { | ||
| const theme = props.theme; | ||
| return { | ||
| display: theme.display, |
There was a problem hiding this comment.
Are these styles based on stas/www-next/styles/src/components/label.css ?
.cf-label {
display: inline-block;
padding: $cf-label-padding;
font-size: $cf-label-font-size;
line-height: $cf-label-line-height;
vertical-align: baseline;
white-space: nowrap;
color: $cf-label-color;
font-weight: $cf-weight-semi-bold;
text-transform: uppercase;
-webkit-text-stroke: 0;
border-radius: $cf-label-border-radius;
user-select: none;
}
.cf-label--default {
background: $cf-label-default;
}
.cf-label--info {
background: $cf-label-info;
}
.cf-label--success {
background: $cf-label-success;
}
.cf-label--warning {
background: $cf-label-warning;
}
.cf-label--error {
background: $cf-label-error;
}It seems that's not the case (you used public styles.css?). We are not going to main styles.css anymore. cf-ui/cf-component-label should be identical to labels that you can find on cloudflare.com. Can you please update. The difference isn't big.
There was a problem hiding this comment.
I used the public styles, this was before we decided to have a singular style sheet. I'll put in our private styles.
| import { checkBaseTheme } from 'cf-style-const'; | ||
|
|
||
| export default baseTheme => { | ||
| checkBaseTheme(baseTheme, 'ButtonTheme'); |
| import { applyTheme } from 'cf-style-container'; | ||
| import { Label as LabelUnstyled, LabelTheme } from 'cf-component-label'; | ||
|
|
||
| const Label = applyTheme(LabelUnstyled, LabelTheme); |
There was a problem hiding this comment.
We now export styled components by default. So this should go into ./src/index.js and you should
export { Label, LabelUnstyled, LabelTheme }
|
Should I still make these changes in this PR? Or open a new one with the updated migration changes? |
|
@manatarms Just fix this PR. |
|
@wyuenho I disagree with migrating to ES6 format simply because of future requirements. The idea to use functional components is solely due to simpler mental model not because of speed benefits. IMO, the author who will need use of refs will convert to ES6 easily. |
@sejoker I don't want to see any inconsistency. Needing refs is not a future requirement, it's a requirement now. This is probably something that should have gone into the discussions but didn't. We've talked about this about this time last year. No regression please, functional components don't buy us much now. |
manatarms
force-pushed
the
fela-component-label
branch
from
b046296
to
0e69a20
Compare
|
Please look at this so I can merge it. I've made all the changes as per the discussion #162 |
| @@ -10,6 +10,10 @@ | |||
| "publishConfig": { | |||
| "registry": "http://registry.npmjs.org/" | |||
| }, | |||
| "dependencies": { | |||
| "cf-style-container": "^1.0.0", | |||
There was a problem hiding this comment.
You don't need to change these manually. Lerna would handle this during the release.
There was a problem hiding this comment.
Should I remove these? Or let them be?
|
|
||
| class Label extends React.Component { | ||
| render() { | ||
| return React.createElement( | ||
| this.props.tagName, |
There was a problem hiding this comment.
Why are we changing the API? We use spans everywhere?
There was a problem hiding this comment.
The reason is because I checked our codebase and the only implementation is using span. This also breaks our pattern of having a standardized component, a user could technically render anything using the tagName.
There was a problem hiding this comment.
in the same way className leaks style this was leaking DOM (lol?). If we actually need to switch the underlying HTML tag we should expose it as an enumerated prop not a free form string.
|
|
||
| export default baseTheme => { | ||
| return { | ||
| borderRadius: baseTheme.borderRadius, |
There was a problem hiding this comment.
Ah man. Prettier defaults to 4.
There was a problem hiding this comment.
No code style code review comments! Prettier is god.
|
@wyuenho If this looks okay to you can we merge it? |
Creating this PR because I had done it. We can merge whenever we decide to.