Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(factories): add autoGenerateKey option #2424

Merged
merged 23 commits into from
May 21, 2018

Conversation

noinkling
Copy link
Contributor

@noinkling noinkling commented Jan 11, 2018

See #2418 for more details.

WIP, currently just includes the implementation and a couple of tests.

Can be used to disable automatic key generation for shorthand props with string/number values
when the factory is called (not when it is created).
@codecov-io
Copy link

codecov-io commented Jan 11, 2018

Codecov Report

Merging #2424 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2424      +/-   ##
==========================================
+ Coverage   99.74%   99.74%   +<.01%     
==========================================
  Files         160      160              
  Lines        2771     2773       +2     
==========================================
+ Hits         2764     2766       +2     
  Misses          7        7
Impacted Files Coverage Δ
src/modules/Dropdown/DropdownHeader.js 100% <ø> (ø) ⬆️
src/modules/Tab/Tab.js 100% <ø> (ø) ⬆️
src/views/Item/Item.js 100% <ø> (ø) ⬆️
src/elements/Button/Button.js 100% <ø> (ø) ⬆️
src/views/Statistic/Statistic.js 100% <ø> (ø) ⬆️
src/modules/Embed/Embed.js 100% <ø> (ø) ⬆️
src/collections/Menu/MenuItem.js 100% <ø> (ø) ⬆️
src/modules/Checkbox/Checkbox.js 100% <ø> (ø) ⬆️
src/views/Feed/FeedLabel.js 100% <ø> (ø) ⬆️
src/modules/Dropdown/Dropdown.js 100% <ø> (ø) ⬆️
... and 27 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 cd73084...ce28c67. Read the comment docs.

@@ -395,6 +396,18 @@ describe('factories', () => {
value: 'a string',
mapValueToProps: () => ({ undef: undefined, nil: null, zero: 0, empty: '' }),
})

Copy link
Contributor Author

@noinkling noinkling Jan 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't completely sure if this was a good place to put the tests, but currently 'key' and 'childKey' (further up) are two separate sections that seem to deal exclusively with explicitly provided object/element props, so it didn't feel right putting it into one of those.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's keep all tests for a given option under that describe() block. Ideally, we only describe(key) once.

@noinkling noinkling changed the title Add generateKey option to createShorthand() feat(factories): add generateKey option to createShorthand() Jan 11, 2018

if (!_.isNil(childKey)) {
// apply and consume the childKey
props.key = typeof childKey === 'function' ? childKey(props) : childKey
delete props.childKey
} else if (valIsString || valIsNumber) {
} else if (generateKey && (valIsString || valIsNumber)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently, setting generateKey: false can still result in creating a key if childKey is passed. I think this check can be hoisted up to line 83. If generateKey is false, I would expect the factory to not perform any operation related to keys. What are your thoughts?

Copy link
Contributor Author

@noinkling noinkling Jan 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is how the check was made in the previous implementation (before it was removed). I guess the question is: is there value in being able to disable an explicitly specified childKey in this way? I believe you can already achieve this by passing in overrideProps: { childKey: null }. There's also the discrepancy with key, which bypasses the conditional.

If the logic stays as is, the option could be renamed to autoGenerateKey or allowAutoKey (etc.) to signify the intent better. Alternatively the conditional logic could be rejigged so that passing overrideProps: { key: null } achieves the same thing, giving it better behaviour parity with object/element values (and removing the need for the extra option altogether), but I think it would require us to differentiate between null and undefined (potentially brittle?).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see your point. The shortcoming of using overrideProps is that it would override the user's key also, even when they explicitly define one. I do like autoGenerateKey. This way, it can be set to false and users can still choose to pass a key or not and get intuitive results.

Copy link
Contributor Author

@noinkling noinkling Jan 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The shortcoming of using overrideProps is that it would override the user's key also, even when they explicitly define one.

Ah I overlooked that somehow, however there's still defaultProps which could work:

...
} else if (props.key === undefined && (valIsString || valIsNumber)) {
...
// At call time:
ItemHeader.create(header, { defaultProps: { key: null } })

The obvious downside is that the semantics of that are far less clear to the user compared to a for-purpose boolean flag (and it's a bit verbose).

@noinkling
Copy link
Contributor Author

noinkling commented Jan 17, 2018

I'm trying to modify the implementsShortHandProp test to accept autoGenerateKey, so each component that implements shorthand props can be tested, however the .should.contain() (or .should.containMatchingElement()) assertion doesn't seem to compare keys (I'm guessing because key isn't a normal prop). It's called here:

shallow(element).should[assertMethod](shorthandElement)

I'm not very familiar with enzyme/chai-enzyme, so I'm not sure of the best way to fix this. After looking at the docs the best I can come up with is something like this:

const wrapper = shallow(element)
wrapper.should[assertMethod](shorthandElement)
expect(wrapper.find(ShorthandComponent).key()).to.equal(shorthandElement.key)

But there's an assumption here that .find() will return a single node, and I don't know if that's an ok assumption to make in this context? Also it's essentially doing the work of finding the same node twice. Those could both be avoided if contain()/ containMatchingElement() returned the matched node somehow, but they don't. The extra work could be avoided like so:

const wrapperChild = shallow(element).find(ShorthandComponent)
expect(wrapperChild.equals(shorthandElement)).to.equal(true)
expect(wrapperChild.key()).to.equal(shorthandElement.key)

But that still has the same 1 node assumption, and I don't know how to make it play nice with assertExactMatch since assertMethod isn't used anymore.

Help or guidance welcome.

@noinkling
Copy link
Contributor Author

noinkling commented Jan 25, 2018

So I've implemented the flag for all the shorthand props that already have a test that uses implementsShorthandProp, listed below:

  • Confirm: header, content
  • BreadcrumbDivider: icon
  • FormField: label
  • MenuItem: icon
  • Message: icon, content, header, list
  • Button: icon, label
  • Header: icon, image, subheader
  • Image: label, dimmer
  • Input: action, icon, label, input
  • Label: icon, image, detail
  • Step: icon
  • StepContent: title, description
  • Checkbox: label
  • Dropdown: icon, header
  • DropdownHeader: icon
  • DropdownItem: icon, label, image, flag, description, text
  • Embed: iframe, icon
  • Modal: header, content
  • CardContent: header, meta, description
  • FeedContent: date, summary, extraImages, extraText, meta
  • FeedEvent: icon, image
  • FeedLabel: icon
  • FeedLike: icon
  • FeedMeta: like
  • FeedSummary: date, user
  • Item: image
  • ItemContent: header, meta, description, extra
  • Statistic: label, value

However there are bunch more that aren't specifically tested, at least not using implementsShorthandProp. I haven't touched those ones yet - if they need tests (do they? cc @levithomason) then it's probably best to do that first. Out of the components I've already modified, these are the props I've identified that fall under this category:

  • Confirm: cancelButton, confirmButton
  • Label: removeIcon
  • Step: description, title
  • Dropdown: searchInput
  • Modal: actions, closeIcon
  • FeedLabel: image

Edit: the others that can benefit:

  • Table: headerRow, footerRow
  • TableCell: icon
  • ListContent: header, description
  • ListItem: icon, image, header, description
  • Popup: header, content
  • Progress: label
  • Search: input
  • SearchResult: image
  • Tab: menu
  • Card: image
  • CommentAvatar: src

I think that's all of them.

@levithomason
Copy link
Member

levithomason commented Feb 2, 2018

Yes, I think it is best to update all usage of factories where we are creating a single element to use autoGenerateKey: false. Looking really great so far 👍

@levithomason
Copy link
Member

Fixed merge conflicts here.

@noinkling
Copy link
Contributor Author

noinkling commented May 12, 2018

Thanks. I'm still gradually working on this, it's only taken so long because I'm trying to unify the testing for shorthand patterns that don't fit into the simple case (e.g. nested object keys, collection shorthands). It's mostly done, just need to find the time to do the last little bit.

@levithomason
Copy link
Member

I've fixed the remaining components and merged master. I'm OK merging this now and doubling back for the test updates.

@levithomason levithomason changed the title feat(factories): add generateKey option to createShorthand() feat(factories): add autoGenerateKey option May 18, 2018
@levithomason
Copy link
Member

Merged master and fixed conflicts again. Ready to merge if tests pass.

@levithomason levithomason merged commit 99b6a57 into Semantic-Org:master May 21, 2018
@welcome
Copy link

welcome bot commented May 21, 2018

Congrats on merging your first pull request! 🎉🎉🎉

robot victory dance

@levithomason
Copy link
Member

Released in semantic-ui-react@0.80.1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants