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

refactor: route helper function and add unit tests #40

Merged
merged 3 commits into from
Nov 12, 2024

Conversation

ThomasGross
Copy link
Contributor

Link to issue

https://reload.atlassian.net/browse/DDFBRA-159

Description

Refactored route helper function and add unit tests

@ThomasGross ThomasGross requested review from spaceo and Adamik10 and removed request for spaceo and Adamik10 November 11, 2024 09:27
@spaceo spaceo self-requested a review November 12, 2024 11:54
Copy link
Contributor

@spaceo spaceo left a comment

Choose a reason for hiding this comment

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

Looks good. Nice to have a central function for this.
Approved, but please look at comments 👍

className="block space-y-3 lg:space-y-5"
href={resolveUrl({ type: "work", routeParams: { id: work.workId } })}>
<div>
<div className="relative flex aspect-4/5 h-auto w-full flex-col rounded-base bg-background-overlay px-[15%] pt-[15%]">
Copy link
Contributor

Choose a reason for hiding this comment

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

Just curious:
Why are these classes added here?
I guess they don't have a role in url resolving? :)

// console.log(searchRoute) // Output: /search?query=test

function buildRoute(route: string, params?: RouteParams, query?: QueryParams): string {
export function buildRoute(route: string, params?: RouteParams, query?: QueryParams): string {
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider putting the three parameters in an object, then it is easier to handle optional parameters and you don't need to specify undefined like here:
https://github.com/danskernesdigitalebibliotek/dpl-go/pull/40/files#diff-4e5726f5588ca037aa726de6816a68efcc47e8e4c64ea260599410ee23dcecb1R44

@ThomasGross ThomasGross requested a review from spaceo November 12, 2024 22:55
@ThomasGross ThomasGross merged commit d57d410 into main Nov 12, 2024
8 checks passed
@ThomasGross ThomasGross deleted the DDFBRA-159-global-route-helper-function branch November 12, 2024 22:57
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