Shopify / blockchain-components

Blockchain related React UI components for Shopify storefronts
https://blockchain.shopify.dev
MIT License
81 stars 18 forks source link

:bug: Address issue with lack of keys #203

Closed QuintonC closed 1 year ago

QuintonC commented 1 year ago

ℹ️ What is the context for these changes?

While working with the Tokengate component in a Hydrogen setting, I noticed that there was a violation of lack of unique keys in UnlockingTokens. When I went to resolve the issue I noticed that we were mapping through collections two times before rendering in a TokenList. This changes that implementation to map a single time and return an array of elements instead of an array of objects which are then mapped over to return an array of elements.

🕹️ Demonstration

Errors from before: SCR-20230615-ol4

I'm no longer getting the error when testing the component in Hydrogen.

🎩 How can this be tophatted?

✅ Checklist

QuintonC commented 1 year ago

/snapit

github-actions[bot] commented 1 year ago

🫰✨ Thanks @QuintonC! Your snapshot has been published to npm.

Test the snapshot by updating your package.json with the newly published version:

yarn add @shopify/tokengate@0.0.0-snapshot-20230615225755
QuintonC commented 1 year ago

Overall this looks good.

One potential concern I have is: Doesn't this PR change the public interface of this component? Could it existing integrations to break? If so, we should make that very explicit.

@Soleone no, this doesn't change the props or interface for the component exported from the package. The only component exported is the Tokengate component.

This doesn't change anything for the developer using the package outside of resolving a warning being logged to the console for a key violation.

This affects only internal component markup by adding keys and preventing us mapping a collection twice.

Soleone commented 1 year ago

Okay, perfect, thanks for clarifying, I was just double checking! 👍