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

fix(list): Add uuid for each list item #179

Merged
merged 4 commits into from
Mar 11, 2022

Conversation

yasincaliskan
Copy link
Contributor

@yasincaliskan yasincaliskan commented Jan 14, 2022

Description

  • Install uuid package to generate unique IDs.
  • If not provided listItemKeyGenerator or testid props, generates uuid.
  • Also, generating a key with testid is not best practice. Because if list manipulate (add/remove an item or changing order) causes using wrong keys.

Related Issue: #177

@@ -46,6 +47,8 @@ function List<Item extends any>({

if (listItemKeyGenerator) {
key = listItemKeyGenerator(item, listItemTestId);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should re-arrange the ifs:

   if (listItemKeyGenerator) {
          key = listItemKeyGenerator(item, listItemTestId);
          // @ts-ignore
    }  else if (item && typeof item === "object" && item.id) {
        // @ts-ignore
        key = item.id;
        }  else {
          key = uuidv4();
        }

Because I believe currently we don't use id in any case. We override the first if in 43 in the else in 50.
Also I think it's time to create a util for this and clean up the component function body:

        return <Fragment key={generateListItemKey({
          item, listItemTestId, listItemKeyGenerator
          })}>
          {children(item, listItemTestId, index)}
        </Fragment>;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated it, thank you!

@yasincaliskan yasincaliskan requested a review from jamcry January 14, 2022 16:07
@yasincaliskan yasincaliskan requested a review from mucahit January 25, 2022 13:29
Copy link
Contributor

@mucahit mucahit left a comment

Choose a reason for hiding this comment

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

Looks good to me, thank you!

@yasincaliskan yasincaliskan added the Ready for new release PR is reviewed and ready to be merged label Feb 23, 2022
@mucahit mucahit merged commit b1b17a9 into next-release Mar 11, 2022
@mucahit mucahit deleted the fix/add-uuid-for-each-list-item branch March 11, 2022 13:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ready for new release PR is reviewed and ready to be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants