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

WIP:Results page #34

Merged
merged 5 commits into from
Apr 24, 2024
Merged

WIP:Results page #34

merged 5 commits into from
Apr 24, 2024

Conversation

Pete-Y-CS
Copy link
Collaborator

@Pete-Y-CS Pete-Y-CS commented Apr 23, 2024

#24

src/lib/index.ts Outdated
},
);
return {
netDifference: proposedSum - existingSum,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Given this is derived from these, I would be inclined to remove it but it was convenient to do it this way at first

Copy link
Collaborator

Choose a reason for hiding this comment

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

What about just a function that sums the score of an array? Two calls to that, then the caller can subtract?

src/lib/index.ts Outdated
proposedScores: string[];
};

export function getCompletedForScorecard(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

sharing some functionality from elsewhere in order to reuse it and be consistent across our various areas

<div class="results-grid">
<div
class="header"
style="--background-color: {headerBackgroundColour}; --font-color:{headerFontColour}"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't like how I've ended up doing this. Wanted to make sure we used the same colours here but it doesn't really work nicely since afaik I have to put this in every element that pulls them through. We should maybe integrate sass more and just have some styles we can apply to set a background + font colour

Copy link
Collaborator

Choose a reason for hiding this comment

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

Will look into it, if this is always the same, we shouldn't need vars just to plumb into a svelte style

>
Policy Conflicts
</div>
<div class="grid-box">{policyCheckComplete}</div>
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If I had more time I would do this section like I do for the other rows below. It would make this page less cumbersome and we could easily and neatly switch to a link when there is an action for the user

Net Difference
</div>

<LevelOfServiceResults title={"Safety"} existing={2} proposed={68} />
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As mentioned this is just stubbed out because we don't have, afaik, a neat answer to where these scores are coming from in our representation of the state

Copy link
Collaborator

@dabreegster dabreegster left a comment

Choose a reason for hiding this comment

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

Thanks! Just notes for myself tomorrow, will pick this up then

src/lib/index.ts Outdated
);
}

export function getScorecardDifference(scorecardState: ScorecardState) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Missing return type

src/lib/index.ts Outdated
},
);
return {
netDifference: proposedSum - existingSum,
Copy link
Collaborator

Choose a reason for hiding this comment

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

What about just a function that sums the score of an array? Two calls to that, then the caller can subtract?

$state[scorecardKey].proposedScores[idx] != "",
);
}
//@ts-ignore This is a scorecard state
Copy link
Collaborator

Choose a reason for hiding this comment

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

Some lint taught me that ts-expect-error should be used instead, because ts-ignore can become unused silently

@@ -121,6 +121,7 @@ export interface State {
pathCheck: Scorecard;

pathPlacemakingCheck: Scorecard;
feedback: string;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Also need to change getEmptyState

const headerFontColour = backgroundAndFontCombinations.green.font;
let headerBackgroundColour = backgroundAndFontCombinations.green.background;

const policyCheckComplete: boolean =
Copy link
Collaborator

Choose a reason for hiding this comment

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

Double check these don't need to be reactive -- whole page should be re-evaluated when switching, but double check SK

);
</script>

<Breadcrumbs
Copy link
Collaborator

Choose a reason for hiding this comment

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

Will fix merge

<div class="results-grid">
<div
class="header"
style="--background-color: {headerBackgroundColour}; --font-color:{headerFontColour}"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Will look into it, if this is always the same, we shouldn't need vars just to plumb into a svelte style

Peter York and others added 2 commits April 24, 2024 09:57
First table of  result summary page

Add next steps

Fix broken wording
@dabreegster
Copy link
Collaborator

I got things rebased and passing TS checks. Plenty left to do (started a list in the issue), but solid start -- thanks!

@dabreegster dabreegster merged commit 704e4dc into main Apr 24, 2024
@dabreegster dabreegster deleted the results-page branch April 24, 2024 10:01
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