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

DDFFORM 417 #1076

Merged
merged 10 commits into from
Apr 7, 2024
Merged

DDFFORM 417 #1076

merged 10 commits into from
Apr 7, 2024

Conversation

LasseStaus
Copy link
Contributor

@LasseStaus LasseStaus commented Apr 3, 2024

Link to issue

Jira: DDFFORM-417

This PR depends on two PRs.

Design system PR
CMS PR

Description

This PR adds an app OpeningHours that is used to display the opening hours for a branch.

In order to see data, run the wiremock container. docker compose up in the react repo.

Go through commit by commit and check the code.

I've seperated them for clarity, but they depend on eachother.

Generally just comment on anything you find relevant, but specifically the "Commetns / questions" below.

Screenshot of the result

image

Additional comments or questions

  • Please consider what should be added in terms of the generated hooks/types from the codegen.
  • Please comment on error-handling. Currently i am not handling errors from the query. I did inspiration or other clear approach to doing this from other places in code.
  • Currently i am only fetching 1 week at a time. If you have any thoughts in regards to whether this is too little and should be reimplemented to i.e. 3-5 weeks at a time, comment.
  • The only reason there is an InitialDate in the props for .entry and openinghours.tsx, is to enable setting the defaultValue of the date in storybook, to the middle-week of the mocked API data in wiremock-cms.
    If you see another way of doing this, or think it should be removed, comment.

Copy link
Contributor

@kasperg kasperg left a comment

Choose a reason for hiding this comment

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

👍 Nice changes here. I like your hook implementation.

I think some things here need a second look. This includes handling of end user texts.

src/apps/opening-hours/OpeningHours.dev.tsx Show resolved Hide resolved
src/apps/opening-hours/OpeningHourWeekList.tsx Outdated Show resolved Hide resolved
src/apps/opening-hours/OpeningHours.dev.tsx Outdated Show resolved Hide resolved
src/apps/opening-hours/OpeningHours.entry.tsx Outdated Show resolved Hide resolved
src/apps/opening-hours/OpeningHoursHelpers.ts Outdated Show resolved Hide resolved
src/apps/opening-hours/OpeningHoursHelpers.ts Outdated Show resolved Hide resolved
src/apps/opening-hours/OpeningHoursHelpers.ts Outdated Show resolved Hide resolved
src/apps/opening-hours/OpeningHoursHelpers.ts Outdated Show resolved Hide resolved
src/apps/opening-hours/OpeningHoursHelpers.ts Outdated Show resolved Hide resolved
@kasperg kasperg assigned LasseStaus and unassigned kasperg, rasben and kasperbirch1 Apr 4, 2024
This is the main component and story for the OpeningHours app that displays the opening hours for branches.

Depends on several pother commits for functionality.

DDFFORM-417
This adds the components for display a week of opening hour entries.

Depends on several other commits for functionality.

DDFFORM-417
This controls the logic for organizing the query'ed data and navigating the weeks being displayed.

DDFFORM-417
These are all the helpers used for the openingHours app.

DDFFORM-417
This adds a wiremock for the CMS which can be used to replicate the API structure for testing.

DDFFORM-417
Consider if this data is the data / endpoints we want to go forward for mocked data.

DDFFORM-417
I am unsure, if this should be commit here, or added in in another fashion.

It is generated on the target:
https://raw.githubusercontent.com/danskernesdigitalebibliotek/dpl-cms/opening-hours-api/openapi.json

 Includes not only the necessary GET endpoint etc for this ticket, but also other CRUD.

DDFFORM-417
In a previous commit related to OpeningHours, a global locale is set to danish.

This change made this test fail as the expected string is hardcoded to english.

For now the string is manually changed.

In the future, we should change the tests to run in this locale specifically

DDFFORM-417
@LasseStaus LasseStaus assigned kasperg and unassigned LasseStaus Apr 5, 2024
Copy link
Contributor

@kasperg kasperg left a comment

Choose a reason for hiding this comment

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

👍 LGTM

@kasperg kasperg merged commit fad4bd4 into release/form-sprint-10 Apr 7, 2024
21 of 22 checks passed
@kasperg kasperg deleted the DDFFORM-417 branch April 7, 2024 09:51
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.

4 participants