Skip to content
This repository has been archived by the owner on Oct 10, 2023. It is now read-only.

Fix update props and connectWpPost slug entity resolution #49

Merged
merged 8 commits into from
Feb 14, 2017

Conversation

sdgluck
Copy link
Contributor

@sdgluck sdgluck commented Feb 13, 2017

Fixes bugs identified in #44 by @olapersson that prevented component updating on changed props and connectWpPost components resolving entities by slug.

Extras: locks down deps, shouldUpdate can take state as third arg, small updates to README, and others I have forgotten. 💯

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 60d2812 on fix-update-props into ** on master**.

@sdgluck sdgluck added the bug label Feb 13, 2017
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling efcbbd6 on fix-update-props into ** on master**.

@sdgluck sdgluck changed the title Fix update props Fix update props and connectWpPost slug entity resolution Feb 14, 2017
@sdgluck sdgluck requested a review from rasmuswinter February 14, 2017 13:47
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 907bba9 on fix-update-props into ** on master**.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling ed7e83a on fix-update-props into ** on master**.

const entities = this._getState().wordpress.entities[typeConfig.plural]
return entities[realId] || null

for (let i = 0, len = keys.length; i < len; i++) {
Copy link
Member

Choose a reason for hiding this comment

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

As discussed earlier, can't we use the result of the query to know which ID(s) were returned, and then look them up in the entities array?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Some content types, e.g. tag, don't have an id so for these types we fall back on their slug, and vice versa. Maybe there's a better way still?

Copy link
Member

Choose a reason for hiding this comment

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

But this is part of KasiaConnectWpPostComponent, so won't the data always be a post, so have an id?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, no, sorry... confusingly post is synonymous with content type here.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I didn't realise that. In that case, this should do until (unless) we find a better solution

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling a465d42 on fix-update-props into ** on master**.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 07fb507 on fix-update-props into ** on master**.

@sdgluck sdgluck merged commit ca320f1 into master Feb 14, 2017
@sdgluck sdgluck deleted the fix-update-props branch February 23, 2017 12:34
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants