Iterate the pre-check to include an interoperability check #239
Conversation
mpanne
commented
Dec 16, 2024
mpanne
commented
- Add questions regarding EU interoperability
- Update pre-check landing and result pages to include information about interoperability
- Add page with general information prior to pre-check questions
- Update content of some pre-check pages
mpanne
force-pushed
the
feature_interoperability-pre-check
branch
5 times, most recently
from
7640927 to
a480736
Compare
For smaller screens additionally to progress bar.
For some reason the constant location resulted in failing e2e tests due to not resolvable dependencies in vorpruefung._index.tsx
Additionally to the result for digital rediness aspects.
- including interoperability result - showing all reasons no matter if positive, negative or unsure - show faq
This reverts commit 3405cae.
Additional to the active element, all preceding elements are colored blue too, to better visualise the progress made.
Together with a small refactoring
We might want to revert that once the interoperebility pre-check has been evaluated on staging
- to prevent email app to open when running tests locally - to be able to validate the mail data (working for all browsers)
-removes info "VO tritt ab Januar in Kraft" - changed word from "EU-Länder" to "EU-Mitgliedsstaaten
mpanne
force-pushed
the
feature_interoperability-pre-check
branch
from
1ce6d24 to
9c30b6d
Compare
There was a problem hiding this comment.
Hey, great work! I left a mix of nitpicks and medium proposals, lmk if you need any clarification.
With the possible refactorings and maybe pulling some components out of ResultPage (ReasoningList first and foremost probably), I am starting to think we might get away without the three files and could still merge route.tsx and ResultPage. Interested in your thoughts!
| @@ -26,7 +26,6 @@ jobs: | |||
|
|
|||
| build-and-push-image: | |||
| needs: [check-and-test-shared] | |||
| if: ${{ github.ref == 'refs/heads/main' }} | |||
There was a problem hiding this comment.
Doesn't this lead to PR branches (e.g. by dependabot) being deployed, too?
| const [isExpanded, setIsExpanded] = useState(false); | ||
|
|
||
| return ( | ||
| <details |
There was a problem hiding this comment.
I like the implementation through a details tag!
|
|
||
| export default function AccordionItem(props: AccordionItemProps) { | ||
| const { headline, content, id, boldAppearance } = props; | ||
| const [isExpanded, setIsExpanded] = useState(false); |
There was a problem hiding this comment.
This would allow multiple AccordionItems to be open at the same time, correct? Why do we explicitly need this state here? Shouldn't the native element handle a11y already? Also, have you considered using name to handle the case that only one accordion should be open?
|
|
||
| return ( | ||
| <details | ||
| onToggle={() => setIsExpanded(!isExpanded)} |
There was a problem hiding this comment.
See above comment if this is really necessary.
| id={id} | ||
| > | ||
| <summary | ||
| role="button" |
There was a problem hiding this comment.
Also seems superfluous, but unsure?
| (tuple) => tuple.question.interoperability && tuple.answer === "unsure", | ||
| ); | ||
|
|
||
| function getReasonListItem(data: { |
There was a problem hiding this comment.
As this and the next three functions don't access local variables, I would pull them out of ResultPage and also simplify them to something like
function getReasonListItem(tuple: QuestionAndAnswer) {
const classes = classNames("w-28 h-auto", {
"fill-green-900": tuple.answer === "yes",
"fill-red-900": tuple.answer === "no",
});
const resultType =
tuple.answer === "no" ? "negativeResult" : "positiveResult";
const resultHint = tuple.question?.resultHint?.[resultType];
return (
<>
{tuple.answer === "yes" && <ControlPointOutlined className={classes} />}
{tuple.answer === "no" && <RemoveCircleOutline className={classes} />}
{tuple.answer === "unsure" && <HelpOutline className={classes} />}
<span>
<span>
{tuple.question?.[resultType]}
{resultHint && <RichText markdown={resultHint} />}
</span>
</span>
</>
);
}
function getReasonsList(questions: QuestionAndAnswer[], intro: string) {
return (
questions.length !== 0 && (
<>
<RichText className="mt-40" markdown={intro} />
<ul className="ds-stack-16 mt-16">
{questions
.toSorted((t1) => (t1.answer === "yes" ? -1 : 1))
.map((tuple) => (
<li key={tuple.question.id} className="flex items-start gap-12">
{getReasonListItem(tuple)}
</li>
))}
</ul>
</>
)
);
}
Warning
Not tested!
| } | ||
| > | ||
| <Container backgroundColor="white" additionalClassNames="rounded-b-lg"> | ||
| <div className="pb-40 border-solid border-0 border-b-2 border-gray-400 last:border-0 last:pb-0"> |
There was a problem hiding this comment.
border-0 seems unnecessary here
| [ResultType.NEGATIVE]: preCheck.result.negative.nextSteps, | ||
| }; | ||
|
|
||
| function matchQuestionsAndAnswers(answers: PreCheckAnswers) { |
There was a problem hiding this comment.
How about the following changes in tandem with the lower comments
function getRelevantQuestionsAndAnswers(
answers: PreCheckAnswers,
interoperability: boolean,
sure: boolean,
) {
return questions
.filter(
(q) =>
!!q.interoperability === interoperability &&
(answers[q.id] !== "unsure") === sure,
)
.map((q) => ({ question: q, answer: answers[q.id] }));
}
| answers: PreCheckAnswers; | ||
| result: TResult; | ||
| }>) { | ||
| const questionsAndAnswers = matchQuestionsAndAnswers(answers); |
There was a problem hiding this comment.
const { reasoningIntro } = preCheck.result;
const reasoning = [
{
intro: reasoningIntro.digital.sure,
questions: getRelevantQuestionsAndAnswers(answers, false, true),
},
{
intro: reasoningIntro.digital.unsure,
questions: getRelevantQuestionsAndAnswers(answers, false, false),
},
{
intro: reasoningIntro.interoperability.sure,
questions: getRelevantQuestionsAndAnswers(answers, true, true),
},
{
intro: reasoningIntro.interoperability.unsure,
questions: getRelevantQuestionsAndAnswers(answers, true, false),
},
];
| > | ||
| <Container backgroundColor="white" additionalClassNames="rounded-b-lg"> | ||
| <div className="pb-40 border-solid border-0 border-b-2 border-gray-400 last:border-0 last:pb-0"> | ||
| {getReasonsList( |
There was a problem hiding this comment.
With the above changes the following four elements become just this
{reasoning.map(({ intro, questions }) =>
getReasonsList(questions, intro),
)}
changed instruction text
