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

fix: rebuilds/-styles (and -names) application (pre)view component #1452

Open
wants to merge 14 commits into
base: refactor-seasonal-reservation
Choose a base branch
from

Conversation

vincit-matu
Copy link
Contributor

@vincit-matu vincit-matu commented Oct 11, 2024

🛠️ Changelog

  • Restructures/-styles the application (re)view page
  • ViewInner.tsx becomes ViewApplication.tsx

🧪 Test plan

  • Go to "Omat hakemukset" and select any of your applications to get to the application view page
  • the new styling should match the one in figma
  • The same view should be at the final step when making an application

🎫 Tickets

  • TILA-3573

@vincit-matu vincit-matu force-pushed the update-application-page branch 3 times, most recently from 23e07cb to ad7553d Compare October 11, 2024 14:52
@vincit-matu vincit-matu marked this pull request as ready for review October 11, 2024 20:49
- All secondary buttons / link buttons use white background.
- Remove overrides from buttons inside a Card.
- Replace !important overrides with focus / hover vars.
@joonatank joonatank force-pushed the refactor-seasonal-reservation branch from c7b0105 to dfbf8be Compare October 15, 2024 05:47
@joonatank joonatank force-pushed the refactor-seasonal-reservation branch from dfbf8be to acdb5c3 Compare October 15, 2024 05:55
@@ -51,112 +66,204 @@ const formatDurationSeconds = (seconds: number, t: TFunction): string => {
)}`;
};

const getDuration = (begin: number, end: number, t: TFunction): string => {
Copy link
Contributor

Choose a reason for hiding this comment

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

getDuration -> formatDurationRange
begin -> beginSecs
end -> endSecs


const sections = aes.map((applicationEvent, i) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Wrap the loop invariant into a component. No 100s lines of JSX inside map invariant

const InfoListItem = ({ label, value }: { label: string; value: string }) => (
<li>
{`${label}: `}
<span className="value">{value}</span>
Copy link
Contributor

Choose a reason for hiding this comment

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

why class?

Comment on lines +192 to +197
"status": {
"UNALLOCATED": "Vastaanotettu",
"IN_ALLOCATION": "Käsittelyssä",
"HANDLED": "Hyväksytty",
"REJECTED": "Hylätty"
}
Copy link
Contributor

@joonatank joonatank Oct 16, 2024

Choose a reason for hiding this comment

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

move enum translations to top level of the json with a key that matches the enum type name, not a generic status key

Copy link
Contributor

Choose a reason for hiding this comment

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

Where are the en and sv translations?

Copy link
Contributor

Choose a reason for hiding this comment

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

where are the en and sv translations?

<Accordion heading={t("application:preview.reservationUnitTerms")} open>
<CompactTermsBox
id="preview.acceptServiceSpecificTerms"
heading=""
Copy link
Contributor

@joonatank joonatank Oct 16, 2024

Choose a reason for hiding this comment

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

you made heading optional, why pass heading="" here?

Comment on lines +191 to +194
{reservationUnits?.map((ru) => (
<li key={ru?.pk}>
{getTranslation(ru ?? {}, "name").trim()}
</li>
Copy link
Contributor

Choose a reason for hiding this comment

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

filterNonNullable instead of ?. and ??

Comment on lines +116 to +123
const reservationUnits =
applicationEvent.reservationUnitOptions?.map((eru, index) => ({
pk: eru.reservationUnit?.pk ?? 0,
priority: index,
nameFi: eru.reservationUnit?.nameFi ?? undefined,
nameSv: eru.reservationUnit?.nameSv ?? undefined,
nameEn: eru.reservationUnit?.nameEn ?? undefined,
})) ?? [];
Copy link
Contributor

Choose a reason for hiding this comment

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

why? why not use reservationUnitOptions.reservationUnit directly? ex

filterNonNullable(applicationEvent.reservationUnitOptions?.flatMap((x) => x. reservationUnit))

prefer filterNonNullable over ?? []

why is index turned into priority? looks wrong. why do we need priority? why isn't it gotten from the backend?

Comment on lines +108 to +115
const primaryTimes =
applicationEvent.suitableTimeRanges
?.filter(filterPrimary)
.map(convertApplicationSchedule) ?? [];
const secondaryTimes =
applicationEvent.suitableTimeRanges
?.filter(filterSecondary)
.map(convertApplicationSchedule) ?? [];
Copy link
Contributor

Choose a reason for hiding this comment

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

filterNonNullable over ?? []

@joonatank joonatank force-pushed the refactor-seasonal-reservation branch 3 times, most recently from 22c0c11 to 9f54174 Compare October 22, 2024 06:11
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