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

feat: add helsinki-benefit amount estimate calculator #1045

Merged
merged 10 commits into from
Oct 7, 2024

Conversation

sirtawast
Copy link
Contributor

@sirtawast sirtawast commented Aug 29, 2024

HL-360 / UHF-10588

What was done

How to install

  • npm install
  • optionally, install test dependencies cd src/js/calculator/helsinki_benefit_amount_estimate/tests && npm install

How to test

To run manual tests

To run automatic browser tests

This requires test dependencies to be installed

  • cd src/js/calculator/helsinki_benefit_amount_estimate/tests && npm run test

Continuous documentation

  • This feature has been documented/the documentation has been updated
  • This change doesn't require updates to the documentation

https://helsinkisolutionoffice.atlassian.net/wiki/spaces/HEL/pages/8963751960/Helsinki-lis+n+m+r+n+laskuri

Translations

  • Translations have been added to .js -files and included in this PR

Screenshot 📸 🖼️

screencapture-localhost-3001-src-js-calculator-helsinki-benefit-amount-estimate-helsinki-benefit-test-html-2024-08-29-12_20_08

@sirtawast sirtawast marked this pull request as draft August 29, 2024 09:13
@sirtawast sirtawast force-pushed the benefit-calc branch 3 times, most recently from 61e0a78 to 94d1a83 Compare August 30, 2024 09:06
@sirtawast sirtawast marked this pull request as ready for review September 11, 2024 07:53
@Arkkimaagi Arkkimaagi self-requested a review September 11, 2024 07:55
Copy link
Contributor

@teroelonen teroelonen left a comment

Choose a reason for hiding this comment

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

This is really nicely done and the tests were a good addition. We should definitely consider adding those ourselves! What I would still ask for you to do is to check my comments on the commented lines if they are still needed and to add the calculator machine name to templates/paragraphs/paragraph--calculator.html.twig file on line 32. Also this needs to have the minified js added and committed under the dist folder so please add that as well and we can merge this one 🙏

@sirtawast sirtawast requested a review from teroelonen October 7, 2024 06:54
@sirtawast
Copy link
Contributor Author

This is really nicely done and the tests were a good addition. We should definitely consider adding those ourselves! What I would still ask for you to do is to check my comments on the commented lines if they are still needed and to add the calculator machine name to templates/paragraphs/paragraph--calculator.html.twig file on line 32. Also this needs to have the minified js added and committed under the dist folder so please add that as well and we can merge this one 🙏

Job's done 👍

Copy link
Contributor

@teroelonen teroelonen left a comment

Choose a reason for hiding this comment

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

This is very good now 🦖

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