-
-
Notifications
You must be signed in to change notification settings - Fork 36
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
add reorder component #65
Conversation
…nd layout support
export function removeItem<T>([...arr]: T[], item: T) { | ||
const index = arr.indexOf(item) | ||
index > -1 && arr.splice(index, 1) | ||
return arr |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The removeItem
function uses indexOf
to find the index of the item and splice
to remove it. This approach has a time complexity of O(n) for both indexOf
and splice
, leading to potentially inefficient performance for large arrays. Consider using a more efficient method for removing items, such as filtering the array, which can also achieve O(n) but with a single pass:
export function removeItem<T>(arr: T[], item: T) {
return arr.filter(x => x !== item);
}
export function closestItem<T>(arr: T[], item: T) { | ||
const index = arr.indexOf(item) | ||
if (index === -1) { | ||
return arr[0] | ||
} | ||
else if (index === arr.length - 1) { | ||
return arr[arr.length - 2] | ||
} | ||
else { | ||
return arr[index + 1] | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The closestItem
function does not handle cases where the array has fewer than two elements, which could lead to accessing an undefined index and potentially cause runtime errors. It's important to add checks to ensure the array's length is sufficient before accessing elements by index:
export function closestItem<T>(arr: T[], item: T) {
if (arr.length < 2) return null; // or handle as appropriate
const index = arr.indexOf(item);
if (index === -1) return arr[0];
else if (index === arr.length - 1) return arr[arr.length - 2];
else return arr[index + 1];
}
export const allIngredients = [ | ||
{ icon: '🍅', label: 'Tomato' }, | ||
{ icon: '🥬', label: 'Lettuce' }, | ||
{ icon: '🧀', label: 'Cheese' }, | ||
{ icon: '🥕', label: 'Carrot' }, | ||
{ icon: '🍌', label: 'Banana' }, | ||
{ icon: '🫐', label: 'Blueberries' }, | ||
{ icon: '🥂', label: 'Champers?' }, | ||
] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using emojis as icons in allIngredients
might limit accessibility and internationalization. Consider using image paths or SVGs instead, which can be more flexible and accessible. For example:
{ icon: '/path/to/tomato.svg', label: 'Tomato' }
This approach allows for better control over the presentation and accessibility features like alt
text.
return allIngredients.find(ingredient => !existing.has(ingredient.label)) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The getNextIngredient
function returns undefined
if all ingredients are already included in the input list. This behavior should be explicitly handled or documented to avoid potential runtime errors. Consider adding a check or a default return value to handle this scenario more gracefully. For example:
if (allIngredients.every(ingredient => existing.has(ingredient.label))) {
return defaultIngredient; // or handle the case appropriately
}
export interface LayoutGroupState { | ||
id?: string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The optional properties id
, group
, and forceRender
in the LayoutGroupState
interface could lead to runtime errors if other parts of the application assume these properties are always initialized. Consider ensuring these properties are either always provided or properly handled as optional throughout the application.
Recommended Solution:
- Ensure that any usage of
LayoutGroupState
properties checks forundefined
before usage, or initialize them with default values where possible.
export function useInView<T extends Element = any>( | ||
domRef: Ref<T>, | ||
domRef: Ref<T | null>, | ||
options?: Options['inViewOptions'] | Ref<Options['inViewOptions']>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function useInView
accepts a domRef
which is a Vue ref potentially containing a DOM element. However, there is no explicit error handling or validation to check if domRef
is either null
or does not contain a valid DOM element at the time of invocation. This can lead to runtime errors if domRef
is not properly initialized.
Recommendation:
Add a validation step at the beginning of the function to check if domRef
is valid and contains a DOM element. If not, either throw an error or handle this scenario gracefully to prevent further execution with invalid data.
export function removeItem<T>([...arr]: T[], item: T) { | ||
const index = arr.indexOf(item) | ||
index > -1 && arr.splice(index, 1) | ||
return arr |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The removeItem
function uses indexOf
to find the index of the item to be removed and splice
to remove it. This approach has a time complexity of O(n) for both indexOf
and splice
, leading to potentially inefficient performance for large arrays. Consider using a more efficient method for removing items, such as filtering the array, which can also achieve O(n) complexity but with a simpler and potentially faster execution:
export function removeItem<T>(arr: T[], item: T): T[] {
return arr.filter(x => x !== item);
}
export function closestItem<T>(arr: T[], item: T) { | ||
const index = arr.indexOf(item) | ||
if (index === -1) { | ||
return arr[0] | ||
} | ||
else if (index === arr.length - 1) { | ||
return arr[arr.length - 2] | ||
} | ||
else { | ||
return arr[index + 1] | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The closestItem
function does not handle cases where the array has fewer than two elements, which could lead to accessing undefined array elements and potentially cause runtime errors. It's important to add checks to ensure the array's length is sufficient before attempting to access its elements:
export function closestItem<T>(arr: T[], item: T): T | undefined {
if (arr.length < 2) return undefined;
const index = arr.indexOf(item);
if (index === -1) return arr[0];
else if (index === arr.length - 1) return arr[arr.length - 2];
else return arr[index + 1];
}
const [tomato, lettuce, cheese] = allIngredients | ||
export const initialTabs = [tomato, lettuce, cheese] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The initialization of initialTabs
directly depends on the order of elements in allIngredients
, which can lead to errors if the array is modified (elements added, removed, or reordered). To improve robustness, consider using a more dynamic way of selecting initial tabs, such as filtering based on a property or explicitly specifying the desired elements by their properties to avoid dependency on array order.
Suggested Change:
export const initialTabs = allIngredients.filter(ingredient => ['Tomato', 'Lettuce', 'Cheese'].includes(ingredient.label));
export function getNextIngredient( | ||
ingredients: Ingredient[], | ||
): Ingredient | undefined { | ||
const existing = new Set(ingredients.map(ingredient => ingredient.label)) | ||
return allIngredients.find(ingredient => !existing.has(ingredient.label)) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The getNextIngredient
function assumes that all ingredient labels are unique. If duplicate labels exist, this function may not behave as expected. To ensure the function's reliability, consider adding a mechanism to handle or explicitly forbid duplicate labels in the data structure, or modify the function to handle such cases.
Suggested Change:
Add a validation during the initialization of allIngredients
to ensure all labels are unique or adjust the function to manage duplicates appropriately.
this pr add ReorderGroup、ReorderItem component.
fix type #64 .