HWPV-195: View & Edit individual claim page #650

ashleygyngell · 2024-06-18T17:44:12Z

No description provided.

…rison-visits-internal into claim-editing-page

…after being unassigned

psoleckimoj · 2024-07-22T07:37:44Z

app/assets/sass/application.scss

@@ -4,6 +4,7 @@
 // $govuk-compatibility-govukfrontendtoolkit: true;

 $govuk-global-styles: true;
+$moj-images-path: "../../../node_modules/@ministryofjustice/frontend/moj/assets/images/";


All these hardwired references to node_modules will want addressing at some point, it's not the usual way to reference this and should they change the names or folder structure, which they could do as they don't say this is the way to reference the files, then it could all break.

However, this is as it currently is so it's not going to stop me approving 🙂 Possibly one for @simonnebMOJ to add a ticket for separately?

@psoleckimoj Ah yes, the app kept crashing when it wasn't referenced like this so yes def one to have a look at making a ticket for @simonnebMOJ

Same with all the moj component imports a few lines down in this file. For some reason I couldn't import them with just @import'../../../node_modules/@ministryofjustice/frontend/moj/components/all

psoleckimoj · 2024-07-22T07:41:15Z

app/assets/sass/application.scss

+} 
+
+.justify-content-end {


So not keen on this. Rules should reflect the block and what it does, not just be named after a specific CSS statement, better to create or extend the existing rule that this is being applied to. Like view-claim-display-helper is a good name.

psoleckimoj · 2024-07-22T07:41:48Z

app/assets/sass/application.scss

-    background-color: #00823b;
-    color:#00823b;
+    background-color:  $govuk-success-colour;
+    color: $govuk-success-colour
 }


Not a comment on your update, just realised this rule and the one below are exactly the same 😁

psoleckimoj · 2024-07-22T09:21:22Z

app/assets/sass/application.scss

+
+.claim-approve-different-input {
+    margin: 0px;


No need for px etc when the value is 0

psoleckimoj · 2024-07-22T09:21:49Z

app/assets/sass/application.scss

+
+.claim-table-decision-error-margin {
+    margin-top: 30px!important;


Ideally want to try and get around this as !important is not good practice.

I tried a few alternative approaches but to no avail. Rule only gets set when a specific error pops up upon submitting a claim rejection - so hopefully okay as only a rare case?

psoleckimoj · 2024-07-22T09:22:52Z

app/routes/claim/view-claim.js

@@ -452,19 +456,17 @@ function handleError (error, req, res, updateConflict, next) {
  }
 }

-function populateNewData (data, req) {
+function populateNewData (data, req, keepUnassignedChanges) {


Maybe default to false to make it clearer, i.e. keepUnassignedChanges = false

psoleckimoj · 2024-07-22T09:24:11Z

app/routes/claim/view-claim.js

+        if (!data.claim.AssignedTo) {
+          populateNewData(data, req)
+        } else {
+          populateNewData(data, req, true)


One for the original developers not requiring it, this ideally wants to be an object rather than separate args purely because we don't know what true is doing here.

psoleckimoj · 2024-07-22T09:29:03Z

app/views/partials/view-claim/add-deduction.html

+    <div class="claim-table-deduction-section-container">
+      <div>
+        <span class="govuk-!-margin-top-2 govuk-!-margin-right-4 govuk-!-font-weight-bold">Type</span>


Could just put the classes on the div and remove the span? Also if it's bold maybe it's a heading?

psoleckimoj · 2024-07-22T09:29:27Z

app/views/partials/view-claim/add-deduction.html

+    <div class="claim-table-deduction-section-container">
+      <div>
+        <span class="govuk-!-margin-top-2 govuk-!-margin-right-4 govuk-!-font-weight-bold">Amount</span>


psoleckimoj · 2024-07-22T09:30:15Z

app/views/partials/view-claim/add-deduction.html

+          },
+          {
+            html: deductionAmount,


So, going back to the CSS naming, maybe this class should be deduction-amount and just have the values you're setting here?

..and the same for others.

My thinking was - theres quite a few variants of classes i've set up in the claim table that use the GOV design classes, so set them individually in the html for clarity

psoleckimoj · 2024-07-22T09:41:34Z

app/views/partials/view-claim/closed-claim-actions/manage-overpayments.html

+      {% if Claim['IsOverpaid'] != true %}
+        {% set overPaymentTitleText = 'Amount' %}
+        {% set overPaymentInput = 'overpayment-amount' %}
+      {% elseif Claim['IsOverpaid'] == true %}


Things like this can probably just be an else ?

Or flip so you do the true check first, then everything else will set it otherwise.

psoleckimoj · 2024-07-22T09:42:50Z

app/views/partials/view-claim/closed-claim-actions/payout-barcode-expired.html

+    <span class="text-warning">Payout barcode can not be expired on a rejected claim</span>
+  {% else %}
+    <span class="text-pending">This will cause the original payment to be resubmitted (excluding top ups)</span>
+    <br><br>


Ideally want to use CSS for spacing and not <br>

psoleckimoj · 2024-07-22T09:53:42Z

app/views/partials/view-claim/overview-summary.html

+
+{% set visitRules %}
+  <span class="govuk-caption-xl">Visit rules</span>
+  <h1 class="govuk-heading-xl govuk-!-margin-bottom-4">{{ displayHelper.getRegionRulesByValue(Claim['Country']) }}</h1>


Heading needs to be meaningful, so caption text would go inside the header tag and restyle there. Think about what a screenreader would say out loud 🙂

psoleckimoj · 2024-07-22T10:11:44Z

app/views/partials/view-claim/release-date.html

+        ]
+      }) }}
+    </fieldset>
+  {% else %}


Could do an elsif then else

psoleckimoj · 2024-07-22T10:12:44Z

app/views/partials/view-claim/top-ups.html

+
+{% if TopUps.length > 0 %}
+
+{% set tableRows = [


Need some indentation

psoleckimoj · 2024-07-22T10:20:47Z

app/views/partials/view-claim/warnings.html

@@ -1,67 +1,130 @@
+{% from "govuk/components/warning-text/macro.njk" import govukWarningText %}
+
+{# DUPLICATE #}
 {% if duplicates.length > 0 %}


I think you can just do if duplicates | length 🤔

psoleckimoj · 2024-07-22T10:23:50Z

app/views/partials/view-claim/warnings.html

-  </div>
+  {% set disabledReason %}
+    {% if Claim.DisabledReason %}
+      This Reference has been disabled <br>


Could move this line of text before the if as it is always required.

psoleckimoj · 2024-07-22T10:57:40Z

Main comments:

Rather than using HTML <br> etc for layout, try using margins/padding etc on surrounding/containing blocks.
CSS rules, better to extend existing rules or a sub-class (in SASS) than naming a class after the rule it's implementing

psoleckimoj · 2024-07-25T12:49:03Z

app/assets/javascripts/view-claim.js

-      $(this).next('input').on('input').removeClass('approved-amount')
-    } totalApproved()
-  })
+  if ($('#unassign').length) {


Not sure how I missed this, the code inside the if and the else are identical so no need for the statement?

psoleckimoj · 2024-07-25T12:51:14Z

app/assets/javascripts/view-claim.js

+      if (isValidValue(value)) {
+        approvedCost += parseFloat(value)
+      }
+      approvedCost += +$(this).text().replace('£', '').trim()


Actually this doesn't make sense, you're using approvedCost as both a running total but you're also appending string/text to it, then using it as a number on line 32?

psoleckimoj · 2024-07-26T11:22:39Z

app/assets/javascripts/view-claim.js

-      document.getElementById('additional-info-reject-manual').style.display = 'block'
-      document.getElementById('manual-label').style.display = 'block'
-    }
+  function isValidValue (value) {


Sorry I seem to spot things after re-reading. Is value a string or Number? isNan doesn't make sense if you're also using trim on a string? isNaN will be OK with 1e3 for example.

ashleygyngell added 15 commits June 18, 2024 18:43

HWPV-195: WIP update to move to new component library & design tweaks

0bdbc8c

HWPV-195: visitor section

29dcdfb

HWPV-195: bank, benefit & child sections

4411511

HWPV-195: prisoner & release date section

e6ba557

HWPV-195: visit section

dd6a852

HWPV-195 claim section

4cbf08d

HWPV-195 claim section -2

88c1623

HWPV-195: deduction section

66d7afc

HWPV-195: deduction section -2

95fa804

HWPV-195: add note section

cc0586d

HWPV-195: lint

3cb9cbf

HWPV-195: closed claim action sections

9e6e072

HWPV-195: event section + timeline

a3cc71c

HWPV-195: errors for open claim

1844329

HWPV-195: errors for closed claim

f001589

ashleygyngell changed the title ~~HWPV-195: WIP update to move to new component library & design tweaks~~ HWPV-195: Moving 'update claim editing page' to new component library Jul 3, 2024

ashleygyngell added 11 commits July 3, 2024 22:19

Merge branch 'main' of ssh://github.com/ministryofjustice/help-with-p…

3b2beba

…rison-visits-internal into claim-editing-page

HWPV-195: remove manual errors

e2a8ae1

HWPV-195: styling for errors

9fdddff

HWPV-195: warning components

10c4cc8

HWPV-195: refactor for decimal input

0e5e44a

HWPV-195: Logic update for approved claim

7601f0c

HWPV-195: js refactor

0cbda76

HWPV-195: html refactor

5d866ac

HWPV-195: Add release date checkbox

c886749

Merge branch 'main' of ssh://github.com/ministryofjustice/help-with-p…

0c880b5

…rison-visits-internal into claim-editing-page

HWPV-195: updating claim approved costs logic & layout tweak

d9d5aca

ashleygyngell marked this pull request as ready for review July 9, 2024 16:43

HWPV-195: display approved cost logic fix

a23e94c

ashleygyngell marked this pull request as draft July 10, 2024 09:39

HWPV-195: bug fix for expenses still displaying non-submitted values …

422eb74

…after being unassigned

ashleygyngell changed the title ~~HWPV-195: Moving 'update claim editing page' to new component library~~ HWPV-195: View & Edit individual claim page Jul 12, 2024

ashleygyngell marked this pull request as ready for review July 12, 2024 19:00

HWPV-195: lint

a34e1c3

simonnebMOJ requested review from psoleckimoj and tpmcgowan July 15, 2024 09:49

psoleckimoj reviewed Jul 22, 2024

View reviewed changes

HWPV-195: Tidy up following comments

b06ee86

psoleckimoj reviewed Jul 25, 2024

View reviewed changes

psoleckimoj reviewed Jul 26, 2024

View reviewed changes

psoleckimoj added 2 commits August 6, 2024 12:41

Merge branch 'main' into claim-editing-page

4e5bdb2

Merge branch 'main' into claim-editing-page

c2d6a37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HWPV-195: View & Edit individual claim page #650

HWPV-195: View & Edit individual claim page #650

ashleygyngell commented Jun 18, 2024

psoleckimoj Jul 22, 2024

ashleygyngell Jul 23, 2024

psoleckimoj Jul 22, 2024

psoleckimoj Jul 22, 2024

psoleckimoj Jul 22, 2024

psoleckimoj Jul 22, 2024

ashleygyngell Jul 23, 2024

psoleckimoj Jul 22, 2024

psoleckimoj Jul 22, 2024 •

edited

Loading

psoleckimoj Jul 22, 2024

psoleckimoj Jul 22, 2024

psoleckimoj Jul 22, 2024

psoleckimoj Jul 22, 2024

ashleygyngell Jul 24, 2024

psoleckimoj Jul 22, 2024

psoleckimoj Jul 22, 2024

psoleckimoj Jul 22, 2024

psoleckimoj Jul 22, 2024

psoleckimoj Jul 22, 2024

psoleckimoj Jul 22, 2024

psoleckimoj Jul 22, 2024

psoleckimoj Jul 22, 2024

psoleckimoj commented Jul 22, 2024

psoleckimoj Jul 25, 2024

psoleckimoj Jul 25, 2024

psoleckimoj Jul 26, 2024


		.claim-table-decision-error-margin {
		margin-top: 30px!important;

		}

		.justify-content-end {

HWPV-195: View & Edit individual claim page #650

Are you sure you want to change the base?

HWPV-195: View & Edit individual claim page #650

Conversation

ashleygyngell commented Jun 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

psoleckimoj Jul 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

psoleckimoj commented Jul 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

psoleckimoj Jul 22, 2024 •

edited

Loading