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: 1773 milestone calculated and adjusted fields #1805

Merged

Conversation

Sepehr-Sobhani
Copy link
Contributor

@Sepehr-Sobhani Sepehr-Sobhani commented Jul 18, 2023

@Sepehr-Sobhani Sepehr-Sobhani changed the title fix milestone calculated and adjusted fields Fix: 1773 milestone calculated and adjusted fields Jul 18, 2023
Copy link
Collaborator

@BCerki BCerki left a comment

Choose a reason for hiding this comment

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

  1. On the view page, if the calculated values aren't calculated yet, there's nothing (I think we should see "This field cannot be calculated due to lack of information now".
    Form:
    image
    View:
    image

2. I couldn't get Holdback Amount This Milestone to calculate even though as far as I can tell from the tooltip, all the required info was filled out: https://www.loom.com/share/5b03056bc6e54993a23f6f7bbd1e94a9
This is what happens in dev, which doesn't look quite right either: https://www.loom.com/share/0ca0e5b9451142fa954a182b9cb7095a
was missing the report received date

  1. This seemed totally unrelated to your PR so I made a separate ticket: https://app.zenhub.com/workspaces/climate-action-secretariat-60ca4121764d710011481ca2/issues/gh/bcgov/cas-cif/1813

  2. When first creating a project, on the summary page I don't see the adjusted/calculated labels, and the calculated values that aren't calculated show up $0.00 instead of "This field cannot be calculated due to lack of information now."
    image

Comment on lines 62 to 67
<style jsx>{`
div.adjustedValue {
.readOnlyAdjustableCalculatedValueWidgetWrapper {
display: flex;
}
div.better {
.adjustedValue {
display: flex;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I remember talking about this in another PR, so might not be possible, but is there a way to get the calculated and adjusted values on top of each other instead of beside each other?

adjustedNetAmount: 89,
adjustedGrossAmount: 67,
adjustedHoldbackAmount: 91,
it("Displays diffs of the data fields that were updated and shows latest committed values", () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This one and the following one are just moving variables around? (Hard to see if there's a real diff in here)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right this one is hard to follow. Just to clarify, this commit is all about moving variables around to make the test a bit more readable and close to what we do in other tests: 113b773

@Sepehr-Sobhani Sepehr-Sobhani force-pushed the 1773-fix-milestone-calculated-and-adjusted-fields branch from e1f0189 to 1ab16bd Compare July 21, 2023 17:18
Copy link
Collaborator

@BCerki BCerki left a comment

Choose a reason for hiding this comment

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

Looking good! Styling is SOO much better now :)

Comment on lines 18 to 19
<div className={`${uiSchema["ui:classNames"] ?? "definition-container"}`}>
{displayLabel && (
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a great solution, nice work! I think it could be simplified to something like:

```<div className={ uiSchema.calculatedValueFormContextProperty ? "adjustable-calculated-value-widget":"definition-container"}`}>

Then any calculated field will get the style without us having to remember to add classnames to the uiSchema

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great suggestion 🤩

Comment on lines +42 to +45
.adjustedValue {
display: flex;
flex-basis: 100%;
padding-top: 0.5em;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Overall the new styling is a huge improvement so I'd be comfortable approving this as-is, but I did come across some extra spacing when the adjusted value is filled out if you want to keep tweaking:
Screenshot from 2023-07-21 15-43-34
Screenshot from 2023-07-21 15-47-21

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Collaborator

@BCerki BCerki left a comment

Choose a reason for hiding this comment

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

I saw a couple changes in the happo shots that might just be flake? (moving tasklist, space between arrows in the diffs changing)

@Sepehr-Sobhani
Copy link
Contributor Author

I saw a couple changes in the happo shots that might just be flake? (moving tasklist, space between arrows in the diffs changing)

Might be, going to re-run the e2e again.

@Sepehr-Sobhani Sepehr-Sobhani merged commit e82c41c into develop Jul 24, 2023
16 checks passed
@Sepehr-Sobhani Sepehr-Sobhani deleted the 1773-fix-milestone-calculated-and-adjusted-fields branch July 24, 2023 22:13
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