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

Reduce cognitive complexity in Card Builder component #4859

Merged
merged 3 commits into from
Oct 27, 2023

Conversation

ddubson
Copy link
Contributor

@ddubson ddubson commented Oct 8, 2023

This PR partially addresses gh-4842 — cognitive complexity is lowered but not fully eliminated. Since there are a number of changes, it is best to review the changes thus far, and hopefully merge in what has been addressed to this point.

Issue gh-4842 should be kept open when/if this PR is merged. I will continue working on the issue.

Changes

  • Add jsdom to dev dependencies to allow for testing functionality globally infused with objects such as window or dom.
  • Exclude unit test suite from Sonar scanning — at this stage, focus only on production source code.
  • Convert cardBuilderUtils.js to Typescript for stronger typing, bubbling up potential issues early.
  • Moved from cardBuilder.js:
    • Move isResizable to cardBuilderUtils — added test cases.
    • Move getDefaultBackgroundClass to cardBuilderUtils — added test cases.
    • Move isUsingLiveTvNaming to cardBuilderUtils — added test cases.
    • Move getDefaultColorIndex to cardBuilderUtils — added test cases.
  • Excised from buildCard function to reduce cognitive complexity:
    • resolveAction determines the action of the card (play, pause, etc.) — added test cases.
    • resolveMixedShapeByAspectRatio determines the type of mixed shape to use based on aspect ratio — added test cases.
    • resolveCardImageContainerCssClasses determines Card Image container CSS classes — added test cases.
    • resolveCardBoxCssClasses determines Card Box CSS classes — added test cases.
    • resolveCardCssClasses determines Card CSS classes — added test cases.

Issues

Continues work on gh-4842

@ddubson ddubson requested review from a team as code owners October 8, 2023 16:19
@jellyfin-bot jellyfin-bot added the merge conflict Conflicts prevent merging label Oct 10, 2023
@jellyfin-bot

This comment was marked as outdated.

@jellyfin-bot jellyfin-bot removed the merge conflict Conflicts prevent merging label Oct 10, 2023
@ddubson
Copy link
Contributor Author

ddubson commented Oct 10, 2023

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

Merge conflicts addressed.

@thornbill thornbill added the cleanup Cleanup of legacy code or code smells label Oct 16, 2023
@jellyfin-bot

This comment was marked as outdated.

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

sonarcloud bot commented Oct 27, 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
0.0% 0.0% Duplication

@jellyfin-bot
Copy link
Collaborator

Cloudflare Pages deployment

Latest commit 19c48c5
Status ✅ Deployed!
Preview URL https://59b59855.jellyfin-web.pages.dev
Type 🔀 Preview

View build logs
View bot logs

@thornbill thornbill added the typescript PRs or issues relating to TypeScript code label Oct 27, 2023
@thornbill thornbill merged commit 06b991d into jellyfin:master Oct 27, 2023
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleanup Cleanup of legacy code or code smells typescript PRs or issues relating to TypeScript code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants