Skip to content
New issue

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

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

Already on GitHub? # to your account

Changed prop type of uniqueID for Item component. #1586

Merged
merged 1 commit into from
Aug 22, 2019

Conversation

revanth0212
Copy link
Contributor

Description

uniqueID prop check was failing since it was mentioned that uniqueID will be a number but in case of category component which internally uses item, it can be a string as well.

So no more errors like this:

image

Related Issue

None. No issue for this. It is just a dev fix.
Closes nothing.

Verification Steps

No more console errors related to prop types.

Checklist

Check the console for prop types errors in category and product pages.

@revanth0212 revanth0212 added the version: Minor This changeset includes functionality added in a backwards compatible manner. label Aug 21, 2019
@revanth0212 revanth0212 requested a review from sirugh August 21, 2019 19:32
@@ -54,7 +54,8 @@ Item.propTypes = {
item: PropTypes.any.isRequired,
itemIndex: PropTypes.number.isRequired,
render: PropTypes.oneOfType([PropTypes.func, PropTypes.string]).isRequired,
uniqueID: PropTypes.number
uniqueID: PropTypes.oneOfType([PropTypes.number, PropTypes.string])
Copy link
Contributor

Choose a reason for hiding this comment

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

Are the uses of uniqueID within this component capable of handling either?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this uses a set downstream which means we need to make sure to use a single type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, we just use it as key for a component. Since react does not allow users to send a prop called key to its children, I am just passing it down as uniqueID. Once it reaches the Item component, it is used as key which can be number or string.

@PWAStudioBot
Copy link
Contributor

Fails
🚫

No linked issue found. Please link a relevant open issue by adding the text "closes #<issue_number>" in your PR.

Messages
📖

Access a deployed version of this PR here. Make sure to wait for the "pwa-pull-request-deploy" job to complete.

📖 DangerCI Failures related to missing labels/description/linked issues/etc will persist until the next push or next nightly build run (assuming they are fixed).

Generated by 🚫 dangerJS against 58ab4d2

@sirugh
Copy link
Contributor

sirugh commented Aug 21, 2019

Thanks for fixing this :)

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
pkg:peregrine version: Minor This changeset includes functionality added in a backwards compatible manner.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants