Skip to content

coderabbit test #167

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.

Sign up for GitHub

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

Open
Zelig880 wants to merge 1 commit into
base: main
Choose a base branch
from
+2 −2
Open

coderabbit test #167

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions resources/js/pages/library/home.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
<th>Status</th>
<th>Action</th>
</tr>
<tr v-for="(result, index) in borrowed" :key="`br-${index}`">
<tr v-for="(result, index) in borrowed" :key="index">
Copy link
Copy Markdown
Owner Author

@Zelig880 Zelig880 Aug 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@coderabbitai would you expect this to be a good practice in Vue Js? I am not sure if setting the Key the array index is a good idea.

Copy link
Copy Markdown
Owner Author

@Zelig880 Zelig880 Aug 23, 2023

Choose a reason for hiding this comment

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

@coderabbitai would you expect this to be a good practice in Vue Js? I am not sure if setting the Key to the array index is a good idea.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot Aug 23, 2023

Choose a reason for hiding this comment

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

Image description CodeRabbit

@Zelig880, using the index as a key in Vue.js is generally not recommended. The key attribute is used by Vue to track each node's identity and thus reuse and reorder existing elements. When the order of items changes, Vue will try to minimize DOM manipulations by reusing elements with the same key.

If you use the array index as the key and the order of items changes (e.g., an item is added or removed), it can lead to unpredictable behavior because Vue will be reusing elements based on their index rather than their actual identity.

In this case, if borrowed or lent arrays are dynamic and can change over time, it would be better to use a unique identifier from each result object as the key, if one exists. If there isn't a unique identifier, the previous approach of prefixing the index with a string (br- or ld-) is safer than using the index alone, although it still has potential issues if the list can change dynamically.

Here's how you might do it if result objects have a unique id:

@@ -9,7 +9,7 @@
           <th>Status</th>
           <th>Action</th>
         </tr>
-        <tr v-for="(result, index) in borrowed" :key="index">
+        <tr v-for="result in borrowed" :key="result.id">
           <td>{{ result.book.title }}</td>
           <td>{{ result.lend_duration }}</td>
           <td>{{ status[result.status] }}</td>
@@ -26,7 +26,7 @@
           <th>Status</th>
           <th>Action</th>
         </tr>
-        <tr v-for="(result, index) in lent" :key="index">
+        <tr v-for="result in lent" :key="result.id">
           <td>{{ result.book.title }}</td>
           <td>{{ result.lend_duration }}</td>
           <td>{{ status[result.status] }}</td>

This assumes that result.id is a unique identifier for each item in the borrowed and lent arrays.

<td>{{ result.book.title }}</td>
<td>{{ result.lend_duration }}</td>
<td>{{ status[result.status] }}</td>
Expand All @@ -26,7 +26,7 @@
<th>Status</th>
<th>Action</th>
</tr>
<tr v-for="(result, index) in lent" :key="`ld-${index}`">
<tr v-for="(result, index) in lent" :key="index">
<td>{{ result.book.title }}</td>
<td>{{ result.lend_duration }}</td>
<td>{{ status[result.status] }}</td>
Expand Down