cf-component-pagination migration to Fela by sejoker · Pull Request #135 · cloudflare/cf-ui

sejoker · 2017-03-29T16:32:40Z

No description provided.

jwineman · 2017-03-29T17:17:33Z

Looks like cf-builder-pagination tests are failing

…styling before export

tajo

Looking pretty good! I'll test it visually later. We should deal with the tests before merging (I'm working on StyleProvider now)

tajo · 2017-03-30T21:28:59Z

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

 import React from 'react';
 import { Pagination, PaginationItem } from 'cf-component-pagination';
 import Icon from 'cf-component-icon';
+import { applyTheme } from 'cf-style-container';


tajo · 2017-03-30T21:30:46Z

packages/cf-component-pagination/src/Pagination.js

+
+export { PaginationUnstyled };
+
+export default applyTheme(PaginationUnstyled, PaginationTheme);


To preserve the consistency with my code, can you do these exports in index.js instead?

I want consistency with my code, I use default export for PaginationLink and PaginationRoot

tajo · 2017-03-30T21:31:58Z

packages/cf-component-pagination/src/PaginationItem.js

+
+export { PaginationItemUnstyled };
+
+export default applyTheme(PaginationItemUnstyled, PaginationItemTheme);


please, do these exports in index.js

I want consistency with my code, I use default export for PaginationLink and PaginationRoot

tajo · 2017-03-30T21:32:35Z

packages/cf-component-pagination/src/PaginationLink.js

+
+const PaginationLinkUnstyled = createComponent(styles, PaginationLink);
+
+export default applyTheme(PaginationLinkUnstyled, PaginationLinkTheme);


move to index.js

I use export default applyTheme(PaginationLinkUnstyled, PaginationLinkTheme); to import into PaginationItem, I don't want duplication in index.js and PaginationLink

you could import them from ./index.js but that's ok

tajo · 2017-03-30T21:32:48Z

packages/cf-component-pagination/src/PaginationRoot.js

+
+export { PaginationRootUnstyled };
+
+export default applyTheme(PaginationRootUnstyled, PaginationRootTheme);


move to index.js

I use export default applyTheme(PaginationRootUnstyled, PaginationRootTheme) to import into Pagination, I don't want duplication in index.js and PaginationRoot

jamiebuilds · 2017-03-31T00:27:05Z

I agree with @tajo that the export stuff should be done in index.js, if you're super consistent about modules it will make working with the AST much much easier for the styleguide and any transformations/codemods you want to do.

sejoker · 2017-03-31T11:06:56Z

All right, I moved all exports into index.js as per suggestions from @tajo and @thejameskyle. Should be good to go?

jamiebuilds · 2017-03-31T17:09:44Z

packages/cf-builder-pagination/test/PaginationBuilder.js

-test('should should call onPageChange when clicking another page', () => {
-  const onPageChange = createStub();
+// StyleProvider does not pass props
+// test('should call onPageChange when clicking another page', () => {


If you're disabling this test you should use test.skip( instead so that it appears in test results as being skipped. That way you're reminded to revisit it later.

@thejameskyle valid point, updated

sejoker added 10 commits March 30, 2017 15:15

migrate pagination root div into fela

52f8a2c

migrate Pagination to fela

98d50d3

pass PaginationRoot via props int Pagination

9603543

split PaginationLink from PaginationItem

5f349e0

pass PaginationRoot into Pagination inside pagination builder, apply …

27bac60

…styling before export

use capitalize package

f8ec734

upgrade PaginationItem to Fela

ec4b985

migrate PaginationLink to Fela

6195e29

update pagination tests to Fela

729bc89

update test for pagination builder

5d42e2e

sejoker force-pushed the pagination-fela-migration branch from e8cf8c0 to 5d42e2e Compare March 30, 2017 18:30

sejoker added 2 commits March 30, 2017 21:50

update tests

7fe883c

import styled components by default

d26feb7

sejoker changed the title ~~WIP Pagination fela migration~~ Pagination fela migration Mar 30, 2017

sejoker requested review from jamiebuilds, jwineman, manatarms and tajo March 30, 2017 21:22

tajo suggested changes Mar 30, 2017

View reviewed changes

sejoker added 2 commits March 31, 2017 12:04

move applyTheme logic into index.js as per review comments

768378e

removed unused code

9eebc88

jamiebuilds suggested changes Mar 31, 2017

View reviewed changes

skip test with proper comment

f7d6f8b

jamiebuilds approved these changes Mar 31, 2017

View reviewed changes

tajo approved these changes Apr 3, 2017

View reviewed changes

jwineman approved these changes Apr 3, 2017

View reviewed changes

sejoker changed the title ~~Pagination fela migration~~ cf-component-pagination migration to Fela Apr 4, 2017

sejoker merged commit bf4f63a into cloudflare:master Apr 4, 2017

jwineman mentioned this pull request Apr 5, 2017

Fela Migration #100

Closed

33 tasks


		export { PaginationUnstyled };

		export default applyTheme(PaginationUnstyled, PaginationTheme);


		export { PaginationItemUnstyled };

		export default applyTheme(PaginationItemUnstyled, PaginationItemTheme);


		const PaginationLinkUnstyled = createComponent(styles, PaginationLink);

		export default applyTheme(PaginationLinkUnstyled, PaginationLinkTheme);


		export { PaginationRootUnstyled };

		export default applyTheme(PaginationRootUnstyled, PaginationRootTheme);

Conversation

sejoker commented Mar 29, 2017

Uh oh!

jwineman commented Mar 29, 2017

Uh oh!

tajo 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!

jamiebuilds commented Mar 31, 2017

Uh oh!

sejoker commented Mar 31, 2017

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