feat(factories): add overrideProps #1428

layershifter · 2017-03-09T14:42:19Z

Refence: #1424

layershifter · 2017-03-09T14:43:20Z

@levithomason I've added overrideProps to handle situations like with Menu. It would be great to receive your initial feedback there.

levithomason · 2017-03-12T04:45:09Z

I'd like to think about this one a bit.

layershifter · 2017-03-25T19:02:37Z

@levithomason It will be great receive some feedback from you. Of course, when you will have enough time

levithomason · 2017-03-25T19:11:15Z

Whoops, forgot about this thanks! I'm working on the Tabs ATM and also need this feature. Looking now...

levithomason · 2017-03-25T19:19:22Z

src/lib/factories.js

-  return (val, defaultProps) => {
-    return createShorthand(Component, mapValueToProps, val, defaultProps, generateKey)
+  return (val, defaultProps, overrideProps) => {
+    return createShorthand(Component, mapValueToProps, val, defaultProps, overrideProps, generateKey)


The only comment I have is that the arguments here are getting a little wild. There are cases where you'd want to set generateKey but not defaultProps nor overrideProps. That usage is ugly:

createShorthand( Menu, items => ({ items }), null, // defaultProps null, // overrideProps true, // generateKey )

Perhaps we move these optional arguments to an options object?

createShorthand( Menu, items => ({ items }), { generateKey: true } )

EDIT
Fixed completely incorrect signatures.

I don't like functions with more that 3 args, so I agree, it will better.

This would also apply to the returned create factory. Instead of:

Menu.create(menu, null /* defaultProps */, () => /* override props */)

We'd have something like:

Menu.create(menu, { overrideProps: () => { ... } })

layershifter · 2017-03-25T19:22:39Z

I'll rebase it, complete TODOs and update code with your suggestion tomorrow

levithomason · 2017-03-25T19:44:44Z

I'd like to try and simplify the factories as we also add more features. There is only one component that currently requires usages of defaultProps as a function (others use a function but do not need to):

Input.js

    const actionElement = Button.create(action, elProps => ({
      className: cx(
        // all action components should have the button className
        !_.includes(elProps.className, 'button') && 'button',
      ),
    }))

This is ensuring the button className is added, but not duplicated. Currently, the factory merges className but does not check for duplicates, it could check first. Then, we can reduce the defaultProps back to a simple object. If we need a function, there is always overrideProps for all other use cases.

I think we should then also remove the option for defaultProps to be a function and instead use a simple object. Then, we update the current merge className logic to only merge unique classNames.

Thoughts?

codecov-io · 2017-03-27T11:41:12Z

Codecov Report

Merging #1428 into master will decrease coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1428      +/-   ##
==========================================
- Coverage   99.74%   99.74%   -0.01%     
==========================================
  Files         141      141              
  Lines        2384     2383       -1     
==========================================
- Hits         2378     2377       -1     
  Misses          6        6

Impacted Files	Coverage Δ
src/modules/Dropdown/DropdownItem.js	`100% <ø> (ø)`	⬆️
src/collections/Message/Message.js	`100% <ø> (ø)`	⬆️
src/elements/Button/Button.js	`100% <100%> (ø)`	⬆️
src/modules/Modal/Modal.js	`100% <100%> (ø)`	⬆️
src/collections/Breadcrumb/Breadcrumb.js	`100% <100%> (ø)`	⬆️
src/elements/Input/Input.js	`100% <100%> (ø)`	⬆️
src/collections/Breadcrumb/BreadcrumbDivider.js	`100% <100%> (ø)`	⬆️
src/collections/Message/MessageItem.js	`100% <100%> (ø)`	⬆️
src/collections/Table/TableCell.js	`100% <100%> (ø)`	⬆️
src/collections/Table/TableRow.js	`100% <100%> (ø)`	⬆️
... and 11 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f796757...28ee505. Read the comment docs.

levithomason · 2017-03-28T23:13:31Z

Just ran into an issue over at https://github.com/Semantic-Org/Semantic-UI-React/pull/1508/files#r108559120. We currently only enable support for keys if a create method is instantiated with generateKey set to true.

Although we know which components we need keys for, consumers can also use create in their own code. We should enable key support in all factories so that any user can map over any create method and have support for keys. Otherwise, some factories will be impossible to use in a map.

levithomason · 2017-03-29T04:25:54Z

Heads up, I'm gonna push a few changes here.

levithomason · 2017-03-29T07:04:53Z

Heads up, I've got more pushing coming. Working on some of the tests.

layershifter · 2017-03-29T07:13:15Z

I've pushed some updates, so only 28 tests failed now 👍

levithomason · 2017-03-29T09:07:24Z

✔ 7118 tests completed
✖ 6 tests failed

Seems we may have an issue with the BreadcrumbDivider test. The common test for a shorthand prop expects the rendered element to contain the shorthand element. However, the breadcrumb divider uses it's icon props to return an icon with the divider class added.

levithomason · 2017-03-29T09:09:03Z

I'll leave this PR alone for now and come back when I can. I also think we'll need to still make overrideProps a function. It would received the merged defaultProps and usersProps. This way, we can wrap an event handler and also still call the user's original event handler.

layershifter · 2017-03-29T17:53:44Z

✔ 7124 tests completed 👍

TODO

make overrideProps a function
update event handlers where we use overrideProps
update tests, add tests for overrideProps

layershifter · 2017-04-02T14:15:43Z

src/collections/Breadcrumb/Breadcrumb.js

-          return `${k}=${typeof v === 'function' ? v.name || 'func' : v}`
-        }).join('|')
-      }
-      childElements.push(BreadcrumbDivider.create({ content: divider, icon, key }))


Darn, this is technically a breaking change. Users who are using objects to define a breadcrumb divider and also not defining a key will now need to add the key. Otherwise, they won't all render.

I misread this initially, we are one using the factory as an object in this case. The key is then required by us, otherwise, we're getting this in the docs for Breadcrumb:

Each child in an array or iterator should have a unique "key" prop. Check the top-level render call using <div>.

layershifter · 2017-04-02T14:17:23Z

src/addons/Confirm/Confirm.js

+      _.invoke(predefinedProps, 'onClick', e, buttonProps)
+      _.invoke(this.props, 'onConfirm', e, this.props)
+    },
+  })


We handle there onClick on Buttons, let's allow user to handle own onClick 👍

layershifter · 2017-04-02T14:18:54Z

src/collections/Breadcrumb/BreadcrumbDivider.js

@@ -26,13 +26,14 @@ function BreadcrumbDivider(props) {
  const rest = getUnhandledProps(BreadcrumbDivider, props)
  const ElementType = getElementType(BreadcrumbDivider, props)

-  const iconElement = Icon.create(icon, { ...rest, className: classes })
-  if (iconElement) return iconElement
+  if (!_.isNil(icon)) return Icon.create(icon, { defaultProps: { ...rest, className: classes } })


Simplified contidition

layershifter

@levithomason I've added some tests and performed some cleanups, seems we can go after review 😄

levithomason · 2017-04-03T00:10:47Z

EDIT
I reverted the removal of keys in shorthand for breadcrumb dividers. I had misread that code the first time.

Original
this looks good, I'm just going to revert the removal of generated keys for BreadcrumbDividers to prevent a breaking change, see https://github.com/Semantic-Org/Semantic-UI-React/pull/1428/files#r109309892.

We've got #1533 to remove all manually generated keys. This way we can consolidate our breaking changes for key into one release.

levithomason · 2017-04-03T00:39:54Z

Rebased to master, fixed breadcrumb divider keys. Also tossed in a fix the for "show HTML" button in examples.

Will merge on pass.

layershifter added the enhancement label Mar 10, 2017

layershifter added the needs engineering label Mar 12, 2017

levithomason reviewed Mar 25, 2017

View reviewed changes

layershifter added needs author feedback and removed needs engineering labels Mar 25, 2017

levithomason mentioned this pull request Mar 29, 2017

chore(tests): fail on console.error for propType warnings #1507

Closed

layershifter commented Apr 2, 2017

View reviewed changes

layershifter added ready for review and removed needs author feedback labels Apr 2, 2017

levithomason force-pushed the fix/override-factories branch from 845944b to b6a2e57 Compare April 3, 2017 00:26

Alexander Fedyashov added 2 commits April 2, 2017 17:28

feat(factories): add overrideProps

40a55cc

wip(factories): perform updates

58d995e

layershifter and others added 14 commits April 2, 2017 17:28

wip(factories): perform updates

a643a21

feat(factories): always generate keys

fdc93b7

fix(mixed): wip on components update

ea835b1

fix(mixed): wip on components update

979e466

fix more tests, add test todos

60fea40

fix(BreadcrumbDivider): use defaultProps in options

f9dc866

fix(FeedExampleShorthand): use unique shorthand strings

aee4db0

fix(FeedExampleExtraImagesShorthand): use unique keys

53f1d81

fix(FeedExampleEventsProp): use unique shorthand

cbfbb22

fix(Dropdown): use defaultProps in create options

538130c

fix(mixed): fix broken tests

5c8e755

feat(mixed): update event handlers

6e279bf

tests(factory): add tests

f718474

tests(mixed): update tests and fixes prop handling

52e6685

levithomason force-pushed the fix/override-factories branch from b6a2e57 to 52e6685 Compare April 3, 2017 00:28

fix(Breadcrumb): generate key for shorthand divider

0eb972f

fix(ComponentExample): fix show html item active color

28ee505

levithomason force-pushed the fix/override-factories branch from b284779 to 28ee505 Compare April 3, 2017 01:27

levithomason merged commit 754416e into master Apr 3, 2017

levithomason deleted the fix/override-factories branch April 3, 2017 04:48

This was referenced Apr 3, 2017

Issue with props merging on Menu #1424

Closed

feat(List): handler for <List selection/> #1530

Merged

levithomason mentioned this pull request Jan 10, 2018

Item: chance of auto-generated key collision when using shorthand props #2418

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(factories): add overrideProps #1428

feat(factories): add overrideProps #1428

layershifter commented Mar 9, 2017

layershifter commented Mar 9, 2017

levithomason commented Mar 12, 2017

layershifter commented Mar 25, 2017

levithomason commented Mar 25, 2017

levithomason Mar 25, 2017 •

edited

Loading

layershifter Mar 25, 2017

levithomason Mar 25, 2017

layershifter commented Mar 25, 2017

levithomason commented Mar 25, 2017

codecov-io commented Mar 27, 2017 •

edited

Loading

levithomason commented Mar 28, 2017

levithomason commented Mar 29, 2017

levithomason commented Mar 29, 2017 •

edited

Loading

layershifter commented Mar 29, 2017

levithomason commented Mar 29, 2017

levithomason commented Mar 29, 2017

layershifter commented Mar 29, 2017 •

edited

Loading

layershifter Apr 2, 2017

levithomason Apr 2, 2017 •

edited

Loading

levithomason Apr 3, 2017

layershifter Apr 2, 2017 •

edited

Loading

layershifter Apr 2, 2017

layershifter left a comment

levithomason commented Apr 3, 2017 •

edited

Loading

levithomason commented Apr 3, 2017

feat(factories): add overrideProps #1428

feat(factories): add overrideProps #1428

Conversation

layershifter commented Mar 9, 2017

layershifter commented Mar 9, 2017

levithomason commented Mar 12, 2017

layershifter commented Mar 25, 2017

levithomason commented Mar 25, 2017

levithomason Mar 25, 2017 • edited Loading

Choose a reason for hiding this comment

layershifter Mar 25, 2017

Choose a reason for hiding this comment

levithomason Mar 25, 2017

Choose a reason for hiding this comment

layershifter commented Mar 25, 2017

levithomason commented Mar 25, 2017

codecov-io commented Mar 27, 2017 • edited Loading

Codecov Report

levithomason commented Mar 28, 2017

levithomason commented Mar 29, 2017

levithomason commented Mar 29, 2017 • edited Loading

layershifter commented Mar 29, 2017

levithomason commented Mar 29, 2017

levithomason commented Mar 29, 2017

layershifter commented Mar 29, 2017 • edited Loading

TODO

layershifter Apr 2, 2017

Choose a reason for hiding this comment

levithomason Apr 2, 2017 • edited Loading

Choose a reason for hiding this comment

levithomason Apr 3, 2017

Choose a reason for hiding this comment

layershifter Apr 2, 2017 • edited Loading

Choose a reason for hiding this comment

layershifter Apr 2, 2017

Choose a reason for hiding this comment

layershifter left a comment

Choose a reason for hiding this comment

levithomason commented Apr 3, 2017 • edited Loading

levithomason commented Apr 3, 2017

levithomason Mar 25, 2017 •

edited

Loading

codecov-io commented Mar 27, 2017 •

edited

Loading

levithomason commented Mar 29, 2017 •

edited

Loading

layershifter commented Mar 29, 2017 •

edited

Loading

levithomason Apr 2, 2017 •

edited

Loading

layershifter Apr 2, 2017 •

edited

Loading

levithomason commented Apr 3, 2017 •

edited

Loading