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

Bugfix for dynamic urls not properly replaces on Search #2124

Merged
merged 2 commits into from
Sep 11, 2024
Merged

Bugfix for dynamic urls not properly replaces on Search #2124

merged 2 commits into from
Sep 11, 2024

Conversation

j3lte
Copy link
Contributor

@j3lte j3lte commented Sep 5, 2024

Thank you for contributing to Homarr! So that your Pull Request can be handled effectively, please populate the following fields (delete sections that are not applicable)

Category

Bugfix

Overview

When introducing the mentioned PR (see below) that helps write dynamic urls, these dynamic urls were not replaced properly in Search. This PR intends to fix that behavior. It uses the proper hook to rewrite the external url for an AppType.

Pull Request

PR: #2024

SeDemal
SeDemal approved these changes Sep 5, 2024
View reviewed changes
Copy link
Collaborator

@SeDemal SeDemal left a comment

Choose a reason for hiding this comment

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

Thanks for noticing the oversight and directly fixing the issue.
lgtm

SeDemal
SeDemal approved these changes Sep 6, 2024
View reviewed changes
Copy link
Collaborator

@SeDemal SeDemal left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Anything else? x)
Weirdly enough I think all those issues were covered in V1.0 when reintegrating the feature.
Good to have it done properly for this version too though.

@j3lte
Copy link
Contributor Author

j3lte commented Sep 9, 2024

@SeDemal nope, this was just something I clearly overlooked when implementing the previous mentioned feature

@SeDemal SeDemal merged commit c81612e into ajnart:dev Sep 11, 2024
2 checks passed
@SeDemal SeDemal mentioned this pull request Sep 15, 2024
Open
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SeDemal SeDemal SeDemal approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@j3lte @SeDemal