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

Parental leave - Review screen action buttons #2572

Merged
merged 26 commits into from
Jan 22, 2021

Conversation

madebynoam
Copy link
Contributor

  1. Connects the InReview action buttons - allowing the user to view or edit the application they submitted.
  2. Update the style of the conclusion screen to latest design.

Screenshots / Gifs

image

image

image

image

Known issues (for next PRs)

  1. Looks like the strings on Contentful are incorrect, we should re-test when Jeremy completes investigating translations.
  2. There is still placeholder data like const otherParentPeriods which will eventually come from the data-provider/api
  3. The Icelandic translations that I've added are need to be reviewed (came from Google Translate).

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • Formatting passes locally with my changes
  • I have rebased against main before asking for a review

@madebynoam madebynoam added the deprecated:automerge (Disabled) Merge this PR as soon as all checks pass label Jan 19, 2021
@madebynoam madebynoam requested review from a team as code owners January 19, 2021 12:10
@madebynoam madebynoam requested review from barabrian, jeremybarbet and baering and removed request for barabrian January 19, 2021 12:10
Copy link
Contributor

@barabrian barabrian left a comment

Choose a reason for hiding this comment

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

Looks good! just some minor comments regarding unnecessary markup

externalData: ['pregnancyStatus', 'parentalLeaves'],
},
read: 'all',
write: 'all',
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm still unfamiliar with the state machine, but does it mean that the employer can write on all his own answers or write on the applicant answers?

Copy link
Member

Choose a reason for hiding this comment

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

In this case it would give the applicant (or whatever role defined in that scope) the rights to read and write all answers in the application.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It should work as the employer only being able to read a certain part, and submit or reject.
On line 216 we set the permission for the Roles.ASSIGNEE which would be the employer:
read: { answers: ['periods'], externalData: ['pregnancyStatus'] },

Then for Roles.APPLICANT we allow them to write/submit (so that they can set it back to 'draft').

@baering is that correct?

@cypress
Copy link

cypress bot commented Jan 20, 2021



Test summary

10 0 0 0


Run details

Project island.is
Status Passed
Commit 195b4d8 ℹ️
Started Jan 22, 2021 12:42 PM
Ended Jan 22, 2021 12:45 PM
Duration 02:23 💡
OS Linux Ubuntu Linux - 20.04
Browser Electron 78

View run in Cypress Dashboard ➡️


This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

@madebynoam
Copy link
Contributor Author

Thanks all for the reviews! Should be ready for another pass now.

@kodiakhq kodiakhq bot merged commit 672abf4 into main Jan 22, 2021
@kodiakhq kodiakhq bot deleted the review-screen-action-buttons branch January 22, 2021 12:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deprecated:automerge (Disabled) Merge this PR as soon as all checks pass
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants