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

✨ Add questionnaire view driven from example yaml #1285

Merged
merged 2 commits into from
Aug 16, 2023

Conversation

ibolton336
Copy link
Member

@ibolton336 ibolton336 commented Aug 11, 2023

Resolves #1275

Screenshot 2023-08-15 at 1 10 00 PM

@codecov
Copy link

codecov bot commented Aug 11, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.01% 🎉

Comparison is base (edf2855) 43.08% compared to head (922a0fd) 43.09%.
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1285      +/-   ##
==========================================
+ Coverage   43.08%   43.09%   +0.01%     
==========================================
  Files         143      143              
  Lines        4296     4297       +1     
  Branches      998      998              
==========================================
+ Hits         1851     1852       +1     
  Misses       2364     2364              
  Partials       81       81              
Flag Coverage Δ
client 43.09% <100.00%> (+0.01%) ⬆️
server ∅ <ø> (∅)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
client/src/app/Paths.ts 90.47% <100.00%> (+0.23%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ibolton336 ibolton336 marked this pull request as ready for review August 11, 2023 21:14
@ibolton336 ibolton336 requested a review from gildub August 11, 2023 21:15
@ibolton336 ibolton336 changed the title :feat: Add questionnaire view driven from example yaml ✨ Add questionnaire view driven from example yaml Aug 11, 2023
@ibolton336 ibolton336 marked this pull request as draft August 14, 2023 18:51
@ibolton336 ibolton336 force-pushed the questionnaire-view branch 2 times, most recently from c08c506 to 0186998 Compare August 15, 2023 17:09
@ibolton336 ibolton336 marked this pull request as ready for review August 15, 2023 17:09
@ibolton336 ibolton336 requested review from sjd78 and gitdallas and removed request for gildub August 15, 2023 17:10
Copy link
Collaborator

@gitdallas gitdallas left a comment

Choose a reason for hiding this comment

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

gave a couple thoughts but lgtm otherwise

const mapRiskToStatus = (risk: string): IconedStatusPreset => {
switch (risk) {
case "green":
return "LowRisk";
Copy link
Collaborator

Choose a reason for hiding this comment

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

if you wanted to not add to the IconedStatus component, you could just return from here...

"green" = <IconedStatus preset="ok" /> (as it already matches that preset)
"unknown" = <IconedStatus preset="unknown" />
"red" = <IconedStatus icon={<TimesCircleIcon />} status="danger" />
"yellow" = <IconedStatus icon={<WarningTriangleIcon />} status="warning" />

and then line 88 could be just { mapRiskToStatus(answer.risk) } .. although maybe rename to getIconByRisk

but more of a matter of preference and if we expect these presets to be reused

Copy link
Member Author

Choose a reason for hiding this comment

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

Done!

className="tab-content-container"
style={{ flex: 1 }}
>
<Table
Copy link
Collaborator

Choose a reason for hiding this comment

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

using the new table, nice.

would it make sense to pull this into its own component?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done - much cleaner page level component now.

@@ -5,6 +5,7 @@ import CheckCircleIcon from "@patternfly/react-icons/dist/esm/icons/check-circle
import TimesCircleIcon from "@patternfly/react-icons/dist/esm/icons/times-circle-icon";
import InProgressIcon from "@patternfly/react-icons/dist/esm/icons/in-progress-icon";
import ExclamationCircleIcon from "@patternfly/react-icons/dist/esm/icons/exclamation-circle-icon";
import WarningTriangleIcon from "@patternfly/react-icons/dist/esm/icons/warning-triangle-icon";
Copy link
Collaborator

Choose a reason for hiding this comment

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

remnant

switch (risk) {
case "green":
return <IconedStatus preset="Ok" />;
case "unknown":
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit really but could probably just let this fall into default

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

Add answer table

Add tags to be applied labels for questions with conditional tags

Fix weight icons and status

Signed-off-by: ibolton336 <[email protected]>
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.

Questionnaire view
2 participants