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.

Sign up for GitHub

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

Jump to bottom

Refactor home screen #4825

Merged
merged 3 commits into from
Oct 7, 2023
Merged

Refactor home screen #4825

merged 3 commits into from
Oct 7, 2023

Conversation

thornbill
Copy link
Member

Changes
Refactor home screen code to use separate files per home screen section and remove some unused home screen types

Issues
N/A

@thornbill thornbill added typescript PRs or issues relating to TypeScript code cleanup Cleanup of legacy code or code smells labels Oct 2, 2023
@thornbill thornbill requested a review from a team as a code owner October 2, 2023 17:35
ZenenTreadwell
ZenenTreadwell reviewed Oct 3, 2023
View reviewed changes
Copy link

@ZenenTreadwell ZenenTreadwell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome job doing the refactoring work here! I see a couple places where the code could be improved for readability & conforming to React development standards.

Thanks for putting the effort in!

src/components/homesections/sections/liveTv.ts
);
}

function buildSection(
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems rather un-reactive. I'm sure there's a better way to do this with JSX.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The source files these are being split off from are legacy and unfortunately not React. Splitting the legacy pieces out this way should make it easier to divide work on migrating to React going forward, though!

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops! thanks for saying something

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @grhallenbeck that is exactly the idea! I just had not made it back to this to comment myself. 😅

src/components/homesections/sections/recentlyAdded.ts Outdated Show resolved Hide resolved
@jellyfin-bot jellyfin-bot added the merge conflict Conflicts prevent merging label Oct 5, 2023
@jellyfin-bot
Copy link
Collaborator

This pull request has merge conflicts. Please resolve the conflicts so the PR can be successfully reviewed and merged.

@jellyfin-bot jellyfin-bot removed the merge conflict Conflicts prevent merging label Oct 5, 2023
@sonarcloud
Copy link

sonarcloud bot commented Oct 5, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
1.5% 1.5% Duplication

@thornbill thornbill merged commit 7dd5e3e into jellyfin:master Oct 7, 2023
19 checks passed
@thornbill thornbill deleted the home-screen branch October 7, 2023 04:02
@bradbeattie
Copy link
Contributor

Regression of #1134 as a result of this change. Would have expected "Recently Added in Movies" to continue to use the Primary image instead of Backdrops.

image

@bradbeattie bradbeattie mentioned this pull request Dec 2, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@grhallenbeck grhallenbeck grhallenbeck left review comments

@ZenenTreadwell ZenenTreadwell ZenenTreadwell left review comments

@grafixeyehero grafixeyehero grafixeyehero approved these changes

Assignees
No one assigned
Labels
cleanup Cleanup of legacy code or code smells typescript PRs or issues relating to TypeScript code
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@thornbill @jellyfin-bot @bradbeattie @grhallenbeck @ZenenTreadwell @grafixeyehero