[7418] TranscodedVideoPicker control#4825
Draft
jvega190 wants to merge 2 commits intocraftercms:developfrom
Draft
[7418] TranscodedVideoPicker control#4825jvega190 wants to merge 2 commits intocraftercms:developfrom
jvega190 wants to merge 2 commits intocraftercms:developfrom
Conversation
60 tasks
…o feature/7418-transcoded-video-picker # Conflicts: # ui/app/src/components/FormsEngine/lib/controlMap.ts # ui/app/src/components/FormsEngine/lib/valueRetrievers.ts
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughAdds a new TranscodedVideoPicker React control and integrates it into the FormsEngine by registering it in the control map (lazy-loaded), switching its value retriever to array-based extraction, and adding a serializer that prepares array values for XML. Changes
Sequence DiagramsequenceDiagram
actor User
participant Picker as TranscodedVideoPicker
participant Store as ReduxStore
participant DialogStack as DialogStack
participant Preview as PreviewDialog
User->>Picker: Click "View" on item
activate Picker
Picker->>Store: dispatch pushDialog({ type: "video", url: item.url })
deactivate Picker
activate Store
Store->>DialogStack: push dialog entry
deactivate Store
activate DialogStack
DialogStack->>Preview: render with provided URL
deactivate DialogStack
activate Preview
Preview->>User: display video preview
deactivate Preview
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip CodeRabbit can use Trivy to scan for security misconfigurations and secrets in Infrastructure as Code files.Add a .trivyignore file to your project to customize which findings Trivy reports. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
ui/app/src/components/FormsEngine/controls/TranscodedVideoPicker.tsx (1)
64-66: Prefer clearing with an empty array instead ofnull.Line 65 sets
null, but this control’s value type is an array (TranscodedVideoPickerProps.value) and serializer flow inui/app/src/components/FormsEngine/lib/valueSerializers.ts:185-190wraps values as{ item: value }. Using[]keeps type/serialization consistent.Proposed fix
const handleRemoveVideos = () => { - setValue(null); + setValue([]); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/src/components/FormsEngine/controls/TranscodedVideoPicker.tsx` around lines 64 - 66, The handleRemoveVideos function currently calls setValue(null) but the control's value type (TranscodedVideoPickerProps.value) is an array and the serializer expects array values (see valueSerializers wrapping at ui/app/src/components/FormsEngine/lib/valueSerializers.ts). Change handleRemoveVideos to clear the value with an empty array instead of null (i.e., call setValue([])) so the value type and serialization remain consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ui/app/src/components/FormsEngine/controls/TranscodedVideoPicker.tsx`:
- Around line 85-88: The Replace IconButton in TranscodedVideoPicker is
clickable when readonly is false but has no onClick; fix by wiring a real
handler or disabling the control: add an optional prop (e.g., onReplace) to
TranscodedVideoPicker and pass it to the IconButton as onClick, and update the
disabled expression to disabled={readonly || !onReplace} so the button is inert
when no handler is provided; ensure the IconButton (the instance rendering
EditOutlined) still respects autoFocus.
- Around line 76-93: The IconButton elements in TranscodedVideoPicker.tsx (the
view button that calls handleViewVideo, the replace EditOutlined button, and the
delete button that invokes handleRemoveVideos) are icon-only and lack accessible
names; add explicit aria-label attributes to each IconButton (e.g.,
aria-label="View", aria-label="Replace", aria-label="Delete") that match the
Tooltip text so assistive technologies can identify the actions, ensuring you
update the three IconButton instances adjacent to the VisibilityOutlinedIcon,
EditOutlined, and DeleteOutlined icons accordingly.
---
Nitpick comments:
In `@ui/app/src/components/FormsEngine/controls/TranscodedVideoPicker.tsx`:
- Around line 64-66: The handleRemoveVideos function currently calls
setValue(null) but the control's value type (TranscodedVideoPickerProps.value)
is an array and the serializer expects array values (see valueSerializers
wrapping at ui/app/src/components/FormsEngine/lib/valueSerializers.ts). Change
handleRemoveVideos to clear the value with an empty array instead of null (i.e.,
call setValue([])) so the value type and serialization remain consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c06f39df-4550-473b-ae86-64f67fc95f0c
📒 Files selected for processing (4)
ui/app/src/components/FormsEngine/controls/TranscodedVideoPicker.tsxui/app/src/components/FormsEngine/lib/controlMap.tsui/app/src/components/FormsEngine/lib/valueRetrievers.tsui/app/src/components/FormsEngine/lib/valueSerializers.ts
Member
Author
|
Datasources design/work pending |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
craftercms/craftercms#7418
Summary by CodeRabbit