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

Use LinkButton instead of dangerouslySetInnerHTML #5226

Merged
merged 10 commits into from
Sep 23, 2024
Merged

Use LinkButton instead of dangerouslySetInnerHTML #5226

merged 10 commits into from
Sep 23, 2024

Conversation

terite
Copy link
Contributor

@terite terite commented Feb 24, 2024

Let's use the component that already exists for similar reasons to here: #5213

@terite terite requested a review from a team as a code owner February 24, 2024 20:31
@sonarcloud SonarCloud
Copy link

sonarcloud bot commented Feb 24, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues

Measures
0 Security Hotspots
No data about Coverage
0.6% Duplication on New Code

See analysis details on SonarCloud

@thornbill
Copy link
Member

👍 I like the direction of both PRs. Using the "dangerous" prop was never intended to be a long term solution. We should probably also migrate to using the react-router <Link> component.

@thornbill thornbill added the cleanup Cleanup of legacy code or code smells label Feb 24, 2024
@terite
Copy link
Contributor Author

terite commented Feb 25, 2024

We should probably also migrate to using the react-router <Link> component.

react-router-dom's` link implementation is a bit more complex than one might expect. I'd rather not swap it in without additional testing. I'd also like to keep this PR more focused.

@terite terite mentioned this pull request Feb 26, 2024
Open
@jellyfin-bot jellyfin-bot added the merge conflict Conflicts prevent merging label Mar 5, 2024
@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 Sep 23, 2024
github-actions[bot]
github-actions bot previously requested changes Sep 23, 2024
View reviewed changes
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

ESLint doesn't pass. Please fix all ESLint issues.

src/components/search/SearchSuggestions.tsx Outdated Show resolved Hide resolved
src/components/search/SearchSuggestions.tsx Outdated Show resolved Hide resolved
@sonarcloud SonarCloud
Copy link

sonarcloud bot commented Sep 23, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

@jellyfin-bot
Copy link
Collaborator

Cloudflare Pages deployment

Latest commit 76bde38e22d1efa4b763764b9241fa6a17f5601d
Status ✅ Deployed!
Preview URL https://fd6c123c.jellyfin-web.pages.dev
Type 🔀 Preview

@thornbill thornbill added this to the v10.10.0 milestone Sep 23, 2024
@thornbill thornbill changed the title use LinkButton instead of dangerouslySetInnerHTML Use LinkButton instead of dangerouslySetInnerHTML Sep 23, 2024
@thornbill thornbill merged commit 3a414a2 into jellyfin:master Sep 23, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thornbill thornbill thornbill approved these changes

@github-actions github-actions[bot] github-actions[bot] left review comments

Assignees
No one assigned
Labels
cleanup Cleanup of legacy code or code smells
Projects
Jellyfin 10.10
Status: Done
Milestone
v10.10.0
Development

Successfully merging this pull request may close these issues.
3 participants
@terite @thornbill @jellyfin-bot