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

feat(mobile): add context menu for video #2876

Merged
merged 7 commits into from
Feb 25, 2025
Merged

feat(mobile): add context menu for video #2876

merged 7 commits into from
Feb 25, 2025

Conversation

lawvs
Copy link
Member

@lawvs lawvs commented Feb 24, 2025

Resolves FOL-1634

Introduce star functionality in image and video context menus, allowing users to star and unstar entries.

Refactor code for improved type safety and clean up imports.

Copy link

vercel bot commented Feb 24, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
follow ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 24, 2025 5:03pm
1 Skipped Deployment
Name Status Preview Comments Updated (UTC)
follow-external-ssr ⬜️ Ignored (Inspect) Visit Preview Feb 24, 2025 5:03pm

@follow-reviewer-bot
Copy link

Suggested PR Title:

feat: enhance media context menus with video support

Change Summary:
This update modifies several context menu components to enhance media handling, specifically adding a new VideoContextMenu for videos. It also integrates entryId into multiple components, improving the interaction between media elements and their related entries. The MediaCarousel component now accepts entryId, allowing the ImageContextMenu to handle starring entries appropriately. Other components have been cleaned up, removing unused imports and streamlining logic.

Code Review:

Code Review

apps/mobile/src/components/ui/image/ImageContextMenu.tsx

  • Line 62: The feedId is checked at runtime, but this could potentially cause runtime errors if useEntry(entryId) fails to resolve the entry. Instead of assuming feedId exists based on the presence of an entry, validate its existence after fetching the entry and provide proper error handling or a fallback mechanism.

apps/mobile/src/modules/context-menu/video.tsx

  • Line 18: Similar to ImageContextMenu.tsx, the feedId is accessed without additional checks beyond the entry existence. Add explicit validation or fallback handling for feedId to avoid potential runtime errors.

apps/mobile/src/modules/entry-list/templates/EntryGridItem.tsx

  • Lines 44–48: The MediaItems component in the FeedViewType.Pictures case is missing an explicit key in the loop to ensure React can uniquely identify elements. If this is being rendered within a loop, each child should have a unique key.
  • Line 79: The onPreview prop for <MediaItems> is being ignored due to the conditional props. This can lead to inconsistencies if onPreview is expected with this usage for other feed views. Ensure the prop is consistently passed where applicable.

packages/utils/src/url-for-video.ts

  • Lines 39–40: The fallback null assignment on attachments.find(...).url is redundant since find already returns undefined when no match is found. Remove the fallback to streamline the code or replace null consistently if required for downstream compatibility.

These issues can potentially introduce runtime bugs or inconsistencies and should be addressed. Let me know if you need further clarification on specific points.

Copy link

linear bot commented Feb 24, 2025

@lawvs lawvs merged commit 6cbdf45 into dev Feb 25, 2025
12 checks passed
@lawvs lawvs deleted the feat/video-context-menu branch February 25, 2025 05:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant