fix: media minor css changes and split media components #458
Conversation
|
@caushcani is attempting to deploy a commit to the Listinai Team on Vercel. A member of the Team first needs to authorize it. |
|
@jamesread and @nevo-david , some small changes. hope this is a good start :) |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
Heya @caushcani , thanks very much for this first PR, I can see you put work into it. Generally for first PRs, it's recommended to keep things small, and we suggest in the contributors guide that if you're changing more than 3 lines you check with someone - this is because changing lots of files at once is more likely to cause conflicts and makes it much harder to review. I actually just closed 2x PRs yesterday because of this, but yours is almost readable, so I'll read it this evening. Did you test it? |
|
Heya, It looks like you didn't test it, look at all those build errors in the job run. Please submit an additional commit to fix those. |
|
@jamesread On MediaBox component, since pages is not used, I removed setPages from useEffect. Maybe later we can decide why we need pages (pagination maybe?). |
…ring for mediaboxmodal
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
WalkthroughThe changes in this pull request primarily involve updates to import paths across various components, reflecting a reorganization of the project structure. Additionally, the Changes
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@A1exKH just committed fixes. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (7)
apps/frontend/src/components/media/media.component.tsx (4)
Line range hint
23-27: Add missing dependencies touseEffecthookThe
useEffecthook depends ongetValues,props.name, andsetCurrentMediawhich are used within the effect but not specified in the dependency array. This can lead to unexpected behavior if these values change.Apply this diff to fix the issue:
useEffect(() => { const settings = getValues()[props.name]; if (settings) { setCurrentMedia(settings); } -}, []); +}, [getValues, props.name]);
Line range hint
28-30: Remove unnecessary dependency fromcloseDesignModal'suseCallbackThe dependency
modalis included in the dependency array but not used within thecloseDesignModalfunction. This is unnecessary and can be removed to prevent potential confusion.Apply this diff to fix the issue:
const closeDesignModal = useCallback(() => { setMediaModal(false); -}, [modal]); +}, []);
Line range hint
31-34: Include missing dependencies inchangeMedia'suseCallbackThe
useCallbackhook forchangeMediais missing dependencies ononChangeandname, which are used inside the callback. This can lead to stale references if these values change.Apply this diff to fix the issue:
const changeMedia = useCallback((m: { path: string; id: string }) => { setCurrentMedia(m); onChange({ target: { name, value: m } }); -}, []); +}, [name, onChange]);
Line range hint
36-38: Fix dependencies inclearMedia'suseCallbackThe
useCallbackhook forclearMediashould includeonChangeandnamein its dependency array since they are used within the callback. Additionally,valueis included in the dependency array but not utilized inside the function.Apply this diff to fix the issue:
const clearMedia = useCallback(() => { setCurrentMedia(undefined); onChange({ target: { name, value: undefined } }); -}, [value]); +}, [name, onChange]);apps/frontend/src/components/launches/add.edit.model.tsx (3)
472-472: Consider enhancing editor container spacingWhile the flex-1 class handles the space distribution, consider adding padding or margin classes to improve the visual spacing around the editor.
-<div className="flex-1"> +<div className="flex-1 space-y-4">
493-496: Enhance error message accessibilityWhile the color change is good, consider adding aria attributes and role for better accessibility.
-<div className="text-red-500 text-[12px] my-[5px]"> +<div + className="text-red-500 text-[12px] my-[5px]" + role="alert" + aria-live="polite" +>
Line range hint
1-724: Consider splitting the component for better maintainabilityThis component has grown quite large and handles multiple responsibilities (post editing, validation, media handling, scheduling). Consider:
- Extracting the validation logic into a separate hook
- Creating smaller sub-components for the editor toolbar and action buttons
- Adding unit tests for the validation logic and scheduling functionality
This would improve maintainability and make the code easier to test.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (13)
.gitignore(1 hunks)apps/frontend/src/components/launches/add.edit.model.tsx(3 hunks)apps/frontend/src/components/launches/bot.picture.tsx(1 hunks)apps/frontend/src/components/launches/helpers/new.image.component.tsx(1 hunks)apps/frontend/src/components/launches/providers/high.order.provider.tsx(2 hunks)apps/frontend/src/components/launches/providers/reddit/subreddit.tsx(1 hunks)apps/frontend/src/components/layout/layout.settings.tsx(1 hunks)apps/frontend/src/components/layout/settings.component.tsx(1 hunks)apps/frontend/src/components/media/media.component.tsx(1 hunks)apps/frontend/src/components/media/mediabox.component.tsx(1 hunks)apps/frontend/src/components/media/mediaboxmodal.component.tsx(1 hunks)apps/frontend/src/components/media/multimedia.component.tsx(1 hunks)apps/frontend/src/components/settings/teams.component.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (4)
- .gitignore
- apps/frontend/src/components/launches/providers/high.order.provider.tsx
- apps/frontend/src/components/launches/bot.picture.tsx
- apps/frontend/src/components/launches/providers/reddit/subreddit.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
apps/frontend/src/components/media/mediaboxmodal.component.tsx
[error] 16-16: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
apps/frontend/src/components/media/multimedia.component.tsx
[error] 120-120: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
apps/frontend/src/components/media/mediabox.component.tsx
[error] 37-37: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (9)
apps/frontend/src/components/media/media.component.tsx (1)
Line range hint 54-58: Use Next.js <Image> component instead of <img>
Using the <Image> component from next/image can optimize image loading, leading to better performance and user experience.
Apply this diff to implement the change:
{ !!currentMedia && (
<div className="my-[20px] cursor-pointer w-[200px] h-[200px] border-2 border-tableBorder">
- <img
+ <Image
className="w-full h-full object-cover"
src={mediaDirectory.set(currentMedia.path)}
onClick={() => window.open(mediaDirectory.set(currentMedia.path))}
+ width={200}
+ height={200}
+ alt="Media"
/>
</div>
) }Note: Ensure to import Image from next/image and provide appropriate width, height, and alt attributes.
apps/frontend/src/components/launches/helpers/new.image.component.tsx (1)
10-10: Import path updated appropriately
The import path for showMediaBox has been updated to reflect its new location. This ensures the component references are correct.
apps/frontend/src/components/media/multimedia.component.tsx (3)
22-26:
Include value in the dependency array of useEffect
The useEffect hook depends on value from props but doesn't include it in the dependency array. This can cause the effect not to run when value changes.
Apply this diff to fix the issue:
useEffect(() => {
if (value) {
setCurrentMedia(value);
}
-}, []);
+}, [value]);Likely invalid or redundant comment.
51-58:
Include missing dependencies in clearMedia's useCallback
The useCallback hook for clearMedia is missing onChange and name in its dependency array. Include them to prevent potential issues with stale values.
Apply this diff to fix the issue:
const clearMedia = useCallback(
(topIndex: number) => () => {
const newMedia = currentMedia?.filter((_, index: number) => index !== topIndex);
setCurrentMedia(newMedia);
onChange({ target: { name, value: newMedia } });
- },
- [currentMedia]
+ },
+ [currentMedia, name, onChange]
);Likely invalid or redundant comment.
34-41:
Include onChange in dependencies of changeMedia's useCallback
The useCallback hook for changeMedia is missing onChange in its dependency array. Including it ensures the callback updates if onChange changes.
Apply this diff to fix the issue:
const changeMedia = useCallback(
(m: { path: string; id: string }) => {
const newMedia = [...(currentMedia || []), m];
setCurrentMedia(newMedia);
onChange({ target: { name, value: newMedia } });
- },
- [currentMedia, name]
+ },
+ [currentMedia, name, onChange]
);Likely invalid or redundant comment.
apps/frontend/src/components/settings/teams.component.tsx (1)
213-213: LGTM: Boolean prop syntax improvement
The change from secondary=true to secondary={true} follows React best practices for boolean prop types.
apps/frontend/src/components/layout/settings.component.tsx (1)
20-20: LGTM: Import path updated for restructured media components
The import path change aligns with the PR's objective of splitting media components for better organization.
apps/frontend/src/components/layout/layout.settings.tsx (1)
39-39: LGTM: Consistent import path update
The import path change maintains consistency with the media component restructuring across the codebase.
apps/frontend/src/components/launches/add.edit.model.tsx (1)
52-52: LGTM: Import path change aligns with component restructuring
The change from absolute to relative import path is consistent with the PR's objective of splitting media components.
| export const ShowMediaBoxModal: FC = () => { | ||
| const [showModal, setShowModal] = useState(false); | ||
| const [callBack, setCallBack] = | ||
| useState<(params: { id: string; path: string }) => void | undefined>(); |
There was a problem hiding this comment.
Clarify the type of callBack state to avoid confusion
Using void inside a union type can be confusing in TypeScript. To improve clarity, wrap the function type in parentheses before unioning with undefined.
Apply this diff to fix the issue:
const [callBack, setCallBack] =
- useState<(params: { id: string; path: string }) => void | undefined>();
+ useState<((params: { id: string; path: string }) => void) | undefined>();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| useState<(params: { id: string; path: string }) => void | undefined>(); | |
| useState<((params: { id: string; path: string }) => void) | undefined>(); |
🧰 Tools
🪛 Biome (1.9.4)
[error] 16-16: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
| const { data, mutate } = useSWR('get-media', loadMedia); | ||
|
|
||
| useEffect(() => { | ||
| if (data?.results && data?.results?.length) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify conditional using optional chaining
You can simplify the conditional check by leveraging optional chaining and directly checking the length property.
Apply this diff to simplify the code:
-if (data?.results && data?.results?.length) {
+if (data?.results?.length) {
setListMedia([...data.results]);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (data?.results && data?.results?.length) { | |
| if (data?.results?.length) { |
🧰 Tools
🪛 Biome (1.9.4)
[error] 37-37: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
| {!!currentMedia && | ||
| currentMedia.map((media: { path: string ; }, index: number) => ( | ||
| <> | ||
| <div className="cursor-pointer w-[40px] h-[40px] border-2 border-tableBorder relative flex"> |
There was a problem hiding this comment.
Add a key prop to elements in list rendering
When rendering a list using map, each element should have a unique key prop to help React identify items correctly during updates.
Apply this diff to fix the issue:
-currentMedia.map((media: { path: string; }, index: number) => (
- <>
+currentMedia.map((media: { path: string; }, index: number) => (
+ <React.Fragment key={media.id || index}>
<div className="cursor-pointer w-[40px] h-[40px] border-2 border-tableBorder relative flex">
{/* ... */}
</div>
- </>
+ </React.Fragment>
))Ensure you import React to use React.Fragment.
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Biome (1.9.4)
[error] 120-120: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
Thanks for updates! |
|
@A1exKH just fixed. |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (5)
apps/frontend/src/components/media/multimedia.component.tsx (1)
110-136: Consider extracting media rendering logicThe media rendering logic is complex enough to warrant its own component for better maintainability and reusability.
Consider creating a separate
MediaItemcomponent:type MediaItemProps = { media: { path: string }; index: number; onClear: (index: number) => void; mediaDirectory: { set: (path: string) => string }; }; const MediaItem: FC<MediaItemProps> = ({ media, index, onClear, mediaDirectory }) => { const handleOpen = () => window.open(mediaDirectory.set(media.path)); return ( <div className="cursor-pointer w-[40px] h-[40px] border-2 border-tableBorder relative flex"> <div onClick={handleOpen}> {media.path.indexOf('mp4') > -1 ? ( <VideoFrame url={mediaDirectory.set(media.path)} /> ) : ( <Image className="w-full h-full object-cover" src={mediaDirectory.set(media.path)} alt={`Media content ${index + 1}`} width={50} height={50} /> )} </div> <div onClick={() => onClear(index)} className="rounded-full w-[15px] h-[15px] bg-red-800 text-textColor flex justify-center items-center absolute -right-[4px] -top-[4px]" > x </div> </div> ); };🧰 Tools
🪛 Biome (1.9.4)
[error] 112-112: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.(lint/correctness/useJsxKeyInIterable)
apps/frontend/src/components/media/media.component.tsx (4)
31-34: Clarify state variable names for better readabilityThe state variables
modalandmediaModalmay cause confusion due to their similar names. Consider renaming them to more descriptive names to clearly indicate their purpose. For example:
- Rename
modaltoisMediaBoxOpen- Rename
mediaModaltoisDesignModalOpenThis will enhance code readability and maintainability.
Line range hint
56-58: Optimize state update inshowModalThe
showModalfunction toggles themodalstate using!modaland includesmodalin its dependency array. To avoid potential stale state issues and unnecessary dependencies, consider using the functional update form ofsetShowModaland an empty dependency array.Apply this diff to optimize the function:
-const showModal = useCallback(() => { - setShowModal(!modal); -}, [modal]); +const showModal = useCallback(() => { + setShowModal((prev) => !prev); +}, []);
Line range hint
51-54: Update dependency array inchangeMediaThe
changeMediafunction usesnameandonChangefrom props but has an empty dependency array. To ensure the callback updates correctly whennameoronChangechanges, include them in the dependency array.Apply this diff to fix the dependency array:
-const changeMedia = useCallback((m: { path: string; id: string }) => { - setCurrentMedia(m); - onChange({ target: { name, value: m } }); -}, []); +const changeMedia = useCallback((m: { path: string; id: string }) => { + setCurrentMedia(m); + onChange({ target: { name, value: m } }); +}, [name, onChange]);
Line range hint
60-63: Correct dependency array inclearMediaThe
clearMediafunction currently lists[value]in its dependency array, but it usesnameandonChange. To prevent issues with stale references, update the dependency array to includenameandonChange.Apply this diff to fix the dependency array:
-const clearMedia = useCallback(() => { - setCurrentMedia(undefined); - onChange({ target: { name, value: undefined } }); -}, [value]); +const clearMedia = useCallback(() => { + setCurrentMedia(undefined); + onChange({ target: { name, value: undefined } }); +}, [name, onChange]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
apps/frontend/src/components/media/media.component.tsx(2 hunks)apps/frontend/src/components/media/multimedia.component.tsx(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
apps/frontend/src/components/media/multimedia.component.tsx
[error] 112-112: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
🔇 Additional comments (1)
apps/frontend/src/components/media/multimedia.component.tsx (1)
22-27: LGTM!
The state management is clean and appropriate for handling modal visibility and media directory.
| const changeMedia = useCallback( | ||
| (m: { path: string; id: string }) => { | ||
| const newMedia = [...(value || []), m]; | ||
| onChange({ target: { name, value: newMedia } }); | ||
| }, | ||
| [value, name] | ||
| ); | ||
|
|
||
| const showModal = useCallback(() => { | ||
| setShowModal(!modal); | ||
| }, [modal]); | ||
|
|
||
| const closeDesignModal = useCallback(() => { | ||
| setMediaModal(false); | ||
| }, []); | ||
|
|
||
| const clearMedia = useCallback( | ||
| (topIndex: number) => () => { | ||
| const newMedia = value?.filter((f: any, index: number) => index !== topIndex); | ||
| onChange({ target: { name, value: newMedia } }); | ||
| }, | ||
| [value] | ||
| ); |
There was a problem hiding this comment.
Fix missing dependencies in useCallback hooks
The callback functions are missing essential dependencies which could lead to stale closures.
const changeMedia = useCallback(
(m: { path: string; id: string }) => {
const newMedia = [...(value || []), m];
onChange({ target: { name, value: newMedia } });
},
- [value, name]
+ [value, name, onChange]
);
const clearMedia = useCallback(
(topIndex: number) => () => {
const newMedia = value?.filter((f: any, index: number) => index !== topIndex);
onChange({ target: { name, value: newMedia } });
},
- [value]
+ [value, name, onChange]
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const changeMedia = useCallback( | |
| (m: { path: string; id: string }) => { | |
| const newMedia = [...(value || []), m]; | |
| onChange({ target: { name, value: newMedia } }); | |
| }, | |
| [value, name] | |
| ); | |
| const showModal = useCallback(() => { | |
| setShowModal(!modal); | |
| }, [modal]); | |
| const closeDesignModal = useCallback(() => { | |
| setMediaModal(false); | |
| }, []); | |
| const clearMedia = useCallback( | |
| (topIndex: number) => () => { | |
| const newMedia = value?.filter((f: any, index: number) => index !== topIndex); | |
| onChange({ target: { name, value: newMedia } }); | |
| }, | |
| [value] | |
| ); | |
| const changeMedia = useCallback( | |
| (m: { path: string; id: string }) => { | |
| const newMedia = [...(value || []), m]; | |
| onChange({ target: { name, value: newMedia } }); | |
| }, | |
| [value, name, onChange] | |
| ); | |
| const showModal = useCallback(() => { | |
| setShowModal(!modal); | |
| }, [modal]); | |
| const closeDesignModal = useCallback(() => { | |
| setMediaModal(false); | |
| }, []); | |
| const clearMedia = useCallback( | |
| (topIndex: number) => () => { | |
| const newMedia = value?.filter((f: any, index: number) => index !== topIndex); | |
| onChange({ target: { name, value: newMedia } }); | |
| }, | |
| [value, name, onChange] | |
| ); |
| export const MultiMediaComponent: FC<{ | ||
| label: string; | ||
| description: string; | ||
| value?: Array<{ path: string; id: string }>; | ||
| name: string; | ||
| error?: any; | ||
| onChange: (event: { | ||
| target: { name: string; value?: Array<{ id: string; path: string }> }; | ||
| }) => void; | ||
| }> = (props) => { | ||
| const { name, error, onChange, value } = props; | ||
| const user = useUser(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unused props and improve type safety
The component interface has the following issues:
- The
labelanddescriptionprops are declared but never used in the component - The
errorprop uses theanytype which reduces type safety
export const MultiMediaComponent: FC<{
- label: string;
- description: string;
value?: Array<{ path: string; id: string }>;
name: string;
- error?: any;
+ error?: string | null;
onChange: (event: {
target: { name: string; value?: Array<{ id: string; path: string }> };
}) => void;
}> = (props) => {
- const { name, error, onChange, value } = props;
+ const { name, error, onChange, value } = props;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const MultiMediaComponent: FC<{ | |
| label: string; | |
| description: string; | |
| value?: Array<{ path: string; id: string }>; | |
| name: string; | |
| error?: any; | |
| onChange: (event: { | |
| target: { name: string; value?: Array<{ id: string; path: string }> }; | |
| }) => void; | |
| }> = (props) => { | |
| const { name, error, onChange, value } = props; | |
| const user = useUser(); | |
| export const MultiMediaComponent: FC<{ | |
| value?: Array<{ path: string; id: string }>; | |
| name: string; | |
| error?: string | null; | |
| onChange: (event: { | |
| target: { name: string; value?: Array<{ id: string; path: string }> }; | |
| }) => void; | |
| }> = (props) => { | |
| const { name, error, onChange, value } = props; | |
| const user = useUser(); |
| value.map((media: { path: string ; }, index: number) => ( | ||
| <> | ||
| <div className="cursor-pointer w-[40px] h-[40px] border-2 border-tableBorder relative flex"> | ||
| <div | ||
| onClick={() => window.open(mediaDirectory.set(media.path))} | ||
| > | ||
| {media.path.indexOf('mp4') > -1 ? ( | ||
| <VideoFrame url={mediaDirectory.set(media.path)} /> | ||
| ) : ( | ||
| <Image | ||
| className="w-full h-full object-cover" | ||
| src={mediaDirectory.set(media.path)} | ||
| alt={'media'+index} | ||
| width={50} | ||
| height={50} | ||
| /> | ||
| )} | ||
| </div> | ||
| <div | ||
| onClick={clearMedia(index)} | ||
| className="rounded-full w-[15px] h-[15px] bg-red-800 text-textColor flex justify-center items-center absolute -right-[4px] -top-[4px]" | ||
| > | ||
| x | ||
| </div> | ||
| </div> | ||
| </> | ||
| ))} |
There was a problem hiding this comment.
Add key prop to mapped elements and improve accessibility
- Missing key prop in the map iteration
- Alt text for images could be more descriptive
value.map((media: { path: string }, index: number) => (
- <>
+ <React.Fragment key={`media-${index}`}>
<div className="cursor-pointer w-[40px] h-[40px] border-2 border-tableBorder relative flex">
<div onClick={() => window.open(mediaDirectory.set(media.path))}>
{media.path.indexOf('mp4') > -1 ? (
<VideoFrame url={mediaDirectory.set(media.path)} />
) : (
<Image
className="w-full h-full object-cover"
src={mediaDirectory.set(media.path)}
- alt={'media'+index}
+ alt={`Media content ${index + 1}`}
width={50}
height={50}
/>
)}
</div>
{/* ... */}
</div>
- </>
+ </React.Fragment>
))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| value.map((media: { path: string ; }, index: number) => ( | |
| <> | |
| <div className="cursor-pointer w-[40px] h-[40px] border-2 border-tableBorder relative flex"> | |
| <div | |
| onClick={() => window.open(mediaDirectory.set(media.path))} | |
| > | |
| {media.path.indexOf('mp4') > -1 ? ( | |
| <VideoFrame url={mediaDirectory.set(media.path)} /> | |
| ) : ( | |
| <Image | |
| className="w-full h-full object-cover" | |
| src={mediaDirectory.set(media.path)} | |
| alt={'media'+index} | |
| width={50} | |
| height={50} | |
| /> | |
| )} | |
| </div> | |
| <div | |
| onClick={clearMedia(index)} | |
| className="rounded-full w-[15px] h-[15px] bg-red-800 text-textColor flex justify-center items-center absolute -right-[4px] -top-[4px]" | |
| > | |
| x | |
| </div> | |
| </div> | |
| </> | |
| ))} | |
| value.map((media: { path: string ; }, index: number) => ( | |
| <React.Fragment key={`media-${index}`}> | |
| <div className="cursor-pointer w-[40px] h-[40px] border-2 border-tableBorder relative flex"> | |
| <div | |
| onClick={() => window.open(mediaDirectory.set(media.path))} | |
| > | |
| {media.path.indexOf('mp4') > -1 ? ( | |
| <VideoFrame url={mediaDirectory.set(media.path)} /> | |
| ) : ( | |
| <Image | |
| className="w-full h-full object-cover" | |
| src={mediaDirectory.set(media.path)} | |
| alt={`Media content ${index + 1}`} | |
| width={50} | |
| height={50} | |
| /> | |
| )} | |
| </div> | |
| <div | |
| onClick={clearMedia(index)} | |
| className="rounded-full w-[15px] h-[15px] bg-red-800 text-textColor flex justify-center items-center absolute -right-[4px] -top-[4px]" | |
| > | |
| x | |
| </div> | |
| </div> | |
| </React.Fragment> | |
| ))} |
🧰 Tools
🪛 Biome (1.9.4)
[error] 112-112: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
| const closeDesignModal = useCallback(() => { | ||
| setMediaModal(false); | ||
| }, [modal]); |
There was a problem hiding this comment.
Correct dependency arrays in useCallback hooks
The dependency arrays for closeDesignModal and showDesignModal include [modal], but these functions do not use the modal variable internally. This can lead to unnecessary re-creations of these callbacks. The dependency arrays should be adjusted to an empty array [].
Apply this diff to fix the dependency arrays:
-const closeDesignModal = useCallback(() => {
- setMediaModal(false);
-}, [modal]);
+const closeDesignModal = useCallback(() => {
+ setMediaModal(false);
+}, []);
-const showDesignModal = useCallback(() => {
- setMediaModal(true);
-}, [modal]);
+const showDesignModal = useCallback(() => {
+ setMediaModal(true);
+}, []);Also applies to: 47-49
| useEffect(() => { | ||
| const settings = getValues()[props.name]; | ||
| if (settings) { | ||
| setCurrentMedia(settings); | ||
| } | ||
| }, []); |
There was a problem hiding this comment.
Include dependencies in useEffect hook
The useEffect hook has an empty dependency array but uses getValues and props.name. To ensure the effect runs correctly when these values change, include them in the dependency array.
Apply this diff to fix the dependency array:
-useEffect(() => {
+useEffect(() => {
const settings = getValues()[props.name];
if (settings) {
setCurrentMedia(settings);
}
-}, []);
+}, [getValues, props.name]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| useEffect(() => { | |
| const settings = getValues()[props.name]; | |
| if (settings) { | |
| setCurrentMedia(settings); | |
| } | |
| }, []); | |
| useEffect(() => { | |
| const settings = getValues()[props.name]; | |
| if (settings) { | |
| setCurrentMedia(settings); | |
| } | |
| }, [getValues, props.name]); |
|
Hey @caushcani , thanks for your first PR, and welcome to Postiz :-) We really value your time. Hey @A1exKH , haven't seen you around here before, thanks for making some constructive comments on @caushcani 's first PR :-) This is a bit challenging to review as there are a tonne of bot warnings on this, and a lot of pretty big changes. I've just set the CI to approve the build job at least, and I just added a commit for fix a minor merge conflict. |
| setMedia(media); | ||
| closeModal(); | ||
| }, | ||
| [closeModal] |
Check warning
Code scanning / ESLint
verifies the list of dependencies for Hooks like useEffect and similar Warning
| @@ -0,0 +1,142 @@ | |||
| import { FC, useCallback, useEffect, useState } from "react"; | |||
Check warning
Code scanning / ESLint
Disallow unused variables Warning
| const newMedia = [...(value || []), m]; | ||
| onChange({ target: { name, value: newMedia } }); | ||
| }, | ||
| [value, name] |
Check warning
Code scanning / ESLint
verifies the list of dependencies for Hooks like useEffect and similar Warning
| const newMedia = value?.filter((f: any, index: number) => index !== topIndex); | ||
| onChange({ target: { name, value: newMedia } }); | ||
| }, | ||
| [value] |
Check warning
Code scanning / ESLint
verifies the list of dependencies for Hooks like useEffect and similar Warning
|
Hi @jamesread, |
|
This PR is stale because it has been open for 90 days with no activity. |
|
This PR was closed because it has been inactive for 7 days since being marked as stale. |
4 participants
What kind of change does this PR introduce?
Style fix and same component split
Why was this change needed?
Minor style changes, so it looks better and split same component in separate files to make code more readable,
Other information:
eg: Did you discuss this change with anybody before working on it (not required, but can be a good idea for bigger changes). Any plans for the future, etc?
No
Checklist:
Put a "X" in the boxes below to indicate you have followed the checklist;
Summary by CodeRabbit
New Features
MediaBoxcomponent for uploading and selecting media files.ShowMediaBoxModalcomponent to manage the display of the media selection modal.MultiMediaComponentfor managing multimedia content with enhanced user interaction.Bug Fixes
Chores
.gitignoreto exclude theuploadsdirectory.