Convert cf-component-form to Fela by tajo · Pull Request #133 · cloudflare/cf-ui

tajo · 2017-03-29T00:56:58Z

This is ready to go!

sejoker · 2017-04-02T12:17:05Z

packages/cf-component-form/src/Form.js

+});
+
+const addLayoutProp = (children, layout) =>
+  React.Children.map(children, (child, index) => {


nice approach

NVM, I grok this now.

sejoker · 2017-04-02T12:18:57Z

packages/cf-component-form/src/FormFieldset.js

+  borderLeft: theme.main.borderLeft,
+  borderRight: theme.main.borderRight,
+  borderBottom: theme.main.borderBottom,
+  ...clearFix()


sejoker · 2017-04-02T12:19:56Z

packages/cf-component-form/src/FormFieldset.js

+  padding: theme.legend.padding,
+  marginBottom: theme.legend.marginBottom,
+  borderBottom: theme.legend.borderBottom,
+  '@media (min-width: 720px)': layout === 'horizontal'


I like how flexible we could express styling

sejoker

please see my comments

sejoker · 2017-04-02T12:22:56Z

packages/cf-component-form/src/FormHeader.js

+
+const FormHeader = ({ className, title }) => (
+  <div className={className}>
+    <Heading size={4}>


it was h3 before, why use h4 now?

Because it's more similar to the old version where we remove margin when used inside of form (we should not do stuff like this) but our headings come with fixed margin. We most likely make more style changes pretty soon since we don't really use cf-component-form as we should.

Let's just use react-dom here. Also I don't think form header should have any <h*> elements at all, form elements can be used anywhere. If you are within the same outline context, going from <h1> to <h3> it may confuse a11y speech readers.

Just use a div and style it your way.

Done. I still used h3 since it matches the old styles. We will redesign all forms pretty soon anyway (more info coming soon).

sejoker · 2017-04-02T12:25:26Z

packages/cf-style-container/src/index.js

 };

-export { createComponent, applyTheme, ThemeProvider, color };
+const createComponentStyles = (styleFunctions, component) => {


please add a description about use case for this HOC.

Done. I've documented all cf-style-container functions and added a test for createComponentStyles.

wyuenho · 2017-04-03T18:41:22Z

packages/cf-component-form/src/FormHeader.js

+
+const FormHeader = ({ className, title }) => (
+  <div className={className}>
+    <Heading size={4}>


Let's just use react-dom here. Also I don't think form header should have any <h*> elements at all, form elements can be used anywhere. If you are within the same outline context, going from <h1> to <h3> it may confuse a11y speech readers.

Just use a div and style it your way.

wyuenho · 2017-04-03T22:09:31Z

packages/cf-style-container/README.md

+
+### Aliased functions from fela and react-fela
+
+We proxy/alias some useful functions from fela without changing their behaviour. See the original documentation for more details. We wrap all Fela APIs so we can eventually switch Fela to a different CSS in JS lib if ever needed.


I highly doubt we could just switch to any css in js lib but just by wrapping these couple of functions. I have not seen any other css in js lib that does the same thing.

Most of them share the same camelCase CSS in JS styles, so you can use any renderer if there's better. Btw, that's why projects like polished can be shared. It's still nice to have one cf-style-container toolbox.

Polished is just a bunch of "css preprocessor mixins" for CSS-in-JS solutions, basically, just a bunch of simple, basically pure functions. Fela has a lot of behavior that can't be expressed by these simple signatures. Replacing things is never really easy. Back in the days, people thought having an ORM layer would make switch the RDBMS easy, I lol'ed. Anyway, I digress, it's not important.

wyuenho · 2017-04-03T22:37:59Z

packages/cf-component-form/src/FormTheme.js

+  checkBaseTheme(baseTheme, 'FormTheme');
+  return {
+    background: 'white',
+    border: `1px solid ${baseTheme.colorGrayLight}`,


Is there a shorthand plugin that'll allow us to DCE this? Would be nice if we could get the best of both worlds.

Not sure what you mean by this.

Something like expandShorthand({border: ''}) => {borderTop, borderRight...}

We should call this atomization rather than DCE. This would be even better:

...expandBorder('1px solid black')

output

borderTopColor: 'black', borderLeftColor: 'black', borderBottomColor: 'black', borderRightColor: 'black', borderTopColor: 'black', borderLeftStyle: 'solid', borderBottomStyle: 'solid', borderRightStyle: 'solid', borderTopWidth: '1px', borderLeftWidth: '1px', borderBottomWidth: '1px', borderRightWidth: '1px'

wyuenho · 2017-04-03T22:41:17Z

packages/cf-component-form/src/FormLabel.js

-    );
-  }
-}
+const FormLabel = ({ children, className }) => (


Can we get back to classes please? We'll love to have a ref when interoping with other DOM manipulation libs. Also, you'd think this may be faster because of some old blog post and some ancient twitter message, but it actually isn't.

We'll love to have a ref when interoping with other DOM manipulation libs

That sounds like something we definitely want to avoid?

you'd think this may be faster because of some old blog post and some ancient twitter message, but it actually isn't.

It will be with Fiber.

Also, I like them because it's less code to write and it promotes not using internal state.

We'd like to avoid using refs, but can't. Refs also isn't just something used by DOM manip libs too, tho that's a big issue because tooltip and such. Let's wait til fiber.

Well, if that's everyone's opinion you should open a new PR and revert it everywhere since we use functional components within all new cf-ui codebase. I don't like it. If we really need refs, we can do it on per instance basis.

wyuenho · 2017-04-03T22:43:23Z

packages/cf-component-form/src/FormFieldset.js

+
+const FormFieldset = ({ legend, children, styles }) => (
+  <fieldset className={styles.mainStyles}>
+    {' '}


wyuenho · 2017-04-03T22:44:40Z

packages/cf-component-form/example/basic/component.js

-  handleFirstNameChange(firstName) {
-    this.setState({ firstName });
+  handleFirstNameChange(e) {
+    this.setState({ firstName: e.value });


Thanks deity

wyuenho · 2017-04-03T23:08:40Z

packages/cf-style-container/src/index.js

+  connect,
+  combineRules,
+  createComponentStyles
+};


I still don't know why this package is called cf-style-container? It doesn't contain any styles. This looks like cf-util-styles to me.

We wanted to have all style related stuff under cf-style.

Then why is it called container?

because naming is hard, especially at the beginning when you don't exactly know what will go inside, complain to @jwineman, his idea

…provider

…eComponentStyles

jwineman mentioned this pull request Mar 29, 2017

Fela Migration #100

Closed

33 tasks

tajo changed the title ~~[WIP] Convert Form and FormFieldError components to Fela~~ [WIP] Convert cf-component-form to Fela Mar 29, 2017

tajo force-pushed the fela-component-form branch from 1cb46b6 to f65feb6 Compare March 31, 2017 23:14

tajo changed the title ~~[WIP] Convert cf-component-form to Fela~~ Convert cf-component-form to Fela Mar 31, 2017

tajo requested review from hturan, jamiebuilds, jculvey, jwineman, koddsson, manatarms, marksteyn, nkcmr, sejoker, toekneestuck and wyuenho March 31, 2017 23:25

This was referenced Apr 1, 2017

Fix cf-component-button usage in examples #143

Merged

Updates Page and PageHeader components to use Fela #102

Closed

sejoker reviewed Apr 2, 2017

View reviewed changes

sejoker approved these changes Apr 2, 2017

View reviewed changes

jwineman approved these changes Apr 3, 2017

View reviewed changes

wyuenho suggested changes Apr 3, 2017

View reviewed changes

wyuenho reviewed Apr 3, 2017

View reviewed changes

tajo added 6 commits April 4, 2017 13:34

convert Form and FormFieldError components to Fela

0a607c0

migrate FormFieldset to Fela and add new helper function to cf-style-…

879e0aa

…provider

migrate FormFooter to fela

423fa06

add theme for FormHeader

c99a2fe

migrate FormHeader to fela

3b8234f

fix Form tests, update README, example

723e794

tajo added 7 commits April 4, 2017 13:41

remove unused imports

149581f

update /create/ readme of cf-style-container and add a test for creat…

e69b4d2

…eComponentStyles

typo

bf8baeb

update typo in the snapshot

1297297

remove cf-component-heading from the form title

12e5d95

remove random space

2656588

update snapshots

e6d6612

tajo force-pushed the fela-component-form branch from abfc352 to e6d6612 Compare April 4, 2017 20:41

tajo merged commit e630210 into cloudflare:master Apr 4, 2017


		### Aliased functions from fela and react-fela

		We proxy/alias some useful functions from fela without changing their behaviour. See the original documentation for more details. We wrap all Fela APIs so we can eventually switch Fela to a different CSS in JS lib if ever needed.

Conversation

tajo commented Mar 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sejoker left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

tajo commented Mar 29, 2017 •

edited

Loading