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 social entry list component #2733

Merged
merged 11 commits into from
Feb 13, 2025
Merged

feat(mobile): add social entry list component #2733

merged 11 commits into from
Feb 13, 2025

Conversation

lawvs
Copy link
Member

@lawvs lawvs commented Feb 12, 2025

Resolves FOL-1468

This pull request refactors the entry list components by creating separate components for different feed view types and updating the main entry list screen to use these new components.

The most important changes include the creation of EntryListContentArticle, EntryListContentSocial, and the update to EntryListScreen to dynamically select the appropriate component based on the feed view type.

image

Copy link

vercel bot commented Feb 12, 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 13, 2025 3:27pm
1 Skipped Deployment
Name Status Preview Comments Updated (UTC)
follow-external-ssr ⬜️ Ignored (Inspect) Visit Preview Feb 13, 2025 3:27pm

@follow-reviewer-bot
Copy link

Suggested PR Title:

feat(entry-list): introduce article and social entry views

Change Summary:
This PR introduces two new components: EntryListContentArticle and EntryListContentSocial, enhancing the entry list experience for articles and social media feeds. It modifies the main entry list screen to dynamically render these views based on the selected feed type. The changes result in improved maintainability and clearer separation of concerns in the entry list presentation.

Code Review:

Code Review

The changes made here generally look good and reflect a sensible refactor for introducing a modularized entry list system. However, the following issues require attention:

1. File: entry-list-article.tsx

  • Line 42: key usage in the EntryItem component within the renderItem function. The key in React must be unique among siblings. If there is a chance that entryIds might have duplicate values, you should ensure its uniqueness before use, or include a fallback mechanism.
  • Line 50: debouncedFetchEntryContentByStream uses viewableItems.map((item) => item.key). If viewableItems contains an invalid or undefined key, this could cause issues. Verify item.key is always defined, or provide a safe fallback (e.g., a default or logging).

2. File: entry-list-social.tsx

  • Line 142: key usage in the media.map function when rendering images. The image.url is used, but if media includes duplicate or missing url properties, this could generate React key warnings. Ensure that image.url is unique or falls back to a unique identifier (e.g., index).

3. File: entry-list.tsx

  • Line 37: In switch for FeedViewType, the default case is missing. If an unexpected type is received (e.g., a new FeedViewType in the future), it might lead to incorrect behavior. Add a default case to handle unsupported or unknown FeedViewType.

4. General Comments

  • Both the entry-list-article.tsx and entry-list-social.tsx files share a substantial amount of duplicate code, particularly in the FlashList setup and the onViewableItemsChanged logic. Consider extracting common logic into a reusable component or hooks to improve code maintainability and reduce duplication.

Addressing these points will enhance the robustness and maintainability of the codebase.

Copy link

linear bot commented Feb 13, 2025

Copy link

Linting and formatting issues were automatically fixed. Please review the changes.

@lawvs lawvs merged commit 903b989 into dev Feb 13, 2025
11 checks passed
@lawvs lawvs deleted the feat/social-list branch February 13, 2025 16:02
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.

3 participants