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

event search collection replaces a start-param for past with the "now" #134

Merged
merged 3 commits into from
Nov 14, 2023

Conversation

nikomakela
Copy link
Contributor

LIIKUNTA-577

  • upgrade testing-library
  • event search collection replaces a start-param for past with the "now"
  • rearrange the MSW mocks

Map LinkedEvent and CMS queries and responses under their own folders.
Improve the mocks so they would be more reusable.

@nikomakela nikomakela force-pushed the LIIKUNTA-577-no-past-events-carousel branch 2 times, most recently from 612aa13 to 507f955 Compare November 14, 2023 10:41
Comment on lines 5 to 6

export const frontPageCarouselVariables = {
Copy link
Contributor

@karisal-anders karisal-anders Nov 14, 2023

Choose a reason for hiding this comment

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

How about adding some types?

export type FrontPageCarouselVariables = {
  keyword: string;
  location: string;
  division: string;
  sort: string;
  superEventType: string;
  start: string;
  pageSize: number;
  include: string[];
};

export const frontPageCarouselVariables: FrontPageCarouselVariables = {

...

And then dropping the as unknown as Record<string, string> in the other place.
But that brings up the underlying issue that was hidden by the as unknown as Record<string, string>
use i.e. the pageSize being a number and URLSearchParams not accepting that as input:

TS2345: Argument of type  FrontPageCarouselVariables  is not assignable to parameter of type

string | Record<string, string> | string[][] | URLSearchParams
  • Type  FrontPageCarouselVariables  is not assignable to type  Record<string, string> 
    • Property  pageSize  is incompatible with index signature.
      • Type  number  is not assignable to type  string 

So... hmm... is the pageSize being a number acceptable or should it be converted to string or filtered away before giving it to URLSearchParams? How do you see this?

Copy link
Contributor

Choose a reason for hiding this comment

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

If I convert pageSize to string e.g. by:

      const variables = {
        ...frontPageCarouselVariables,
        start: dateInPast,
        pageSize: `${frontPageCarouselVariables.pageSize}`,
      };
      const eventSearchUrl = new URLSearchParams(variables).toString();

then the next error in URLSearchParams is about the include parameter:

Property  include  is incompatible with index signature.
Type  string[]  is not assignable to type  string

Copy link
Contributor

Choose a reason for hiding this comment

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

Anyway, this brings up the question whether the variables passed to URLSearchParams are being handled correctly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's only for mocking and it can really be anything. These were just the parameters that were used in 1 of the examples. I haven't chosen the parameters -- a content manager has. So, I don't want to type it, because it can be just anything. These were the actual parameters for the response that was added there. I think this can be thought as a recorded request.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed the init to URLSearchParams.

@nikomakela nikomakela force-pushed the LIIKUNTA-577-no-past-events-carousel branch 2 times, most recently from f208eb9 to 88109cc Compare November 14, 2023 11:56
LIIKUNTA-577.
Map LinkedEvent and CMS queries and responses under their own folders.
Improve the mocks so they would be more reusable.
@nikomakela nikomakela force-pushed the LIIKUNTA-577-no-past-events-carousel branch from 88109cc to 4717ac9 Compare November 14, 2023 12:24
@nikomakela nikomakela merged commit 98a672e into main Nov 14, 2023
1 check passed
@nikomakela nikomakela deleted the LIIKUNTA-577-no-past-events-carousel branch November 14, 2023 13:30
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.

2 participants