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

[OF#180] Add GovMetric #634

Merged
merged 7 commits into from
Jan 30, 2024
Merged

[OF#180] Add GovMetric #634

merged 7 commits into from
Jan 30, 2024

Conversation

SilviaAmAm
Copy link
Contributor

@SilviaAmAm SilviaAmAm commented Jan 12, 2024

Copy link

codecov bot commented Jan 12, 2024

Codecov Report

Attention: 12 lines in your changes are missing coverage. Please review.

Comparison is base (42b47c4) 73.04% compared to head (8845f0b) 75.12%.
Report is 31 commits behind head on main.

Files Patch % Lines
src/components/AbortionButton/AbortionButton.js 70.00% 6 Missing ⚠️
src/components/ExistingSubmissionOptions.js 57.14% 3 Missing ⚠️
src/components/Form.js 71.42% 2 Missing ⚠️
src/components/FormStart/index.js 50.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #634      +/-   ##
==========================================
+ Coverage   73.04%   75.12%   +2.07%     
==========================================
  Files         219      224       +5     
  Lines        4474     4542      +68     
  Branches     1186     1211      +25     
==========================================
+ Hits         3268     3412     +144     
+ Misses       1167     1091      -76     
  Partials       39       39              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@SilviaAmAm SilviaAmAm force-pushed the feature/180-govmetric-2 branch from f2ef912 to 4794cb2 Compare January 16, 2024 10:42
@SilviaAmAm SilviaAmAm force-pushed the feature/180-govmetric-2 branch from 4794cb2 to 92a5b80 Compare January 16, 2024 14:01
@SilviaAmAm SilviaAmAm force-pushed the feature/180-govmetric-2 branch from 92a5b80 to 8300e5a Compare January 16, 2024 14:07
@SilviaAmAm SilviaAmAm marked this pull request as ready for review January 16, 2024 14:21
Copy link
Contributor

@Viicos Viicos left a comment

Choose a reason for hiding this comment

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

The button seems to appear on the right in the stories:
image

Apart from that, if I understand correctly this also simplifies the current onFormAbort / onLogout logic and everything is centralized in one place?

src/components/AbortionButton/AbortionButton.js Outdated Show resolved Hide resolved
src/components/AbortionButton/AbortionButton.stories.js Outdated Show resolved Hide resolved
src/components/analytics/GovMetricSnippet.js Outdated Show resolved Hide resolved
@SilviaAmAm
Copy link
Contributor Author

The button seems to appear on the right in the stories:

It is because the AbortionButton component returns a toolbar. Maybe this doesn't make much sense and it should return a button and then the component using it can add the toolbar 🤔

Apart from that, if I understand correctly this also simplifies the current onFormAbort / onLogout logic and everything is centralized in one place?

Yes indeed! I tried combining those into one place. There is still the component taking care of saving the submission halfway through that also uses similar code, but I was worried of breaking it, so I didn't refactor it for now.

@SilviaAmAm
Copy link
Contributor Author

SilviaAmAm commented Jan 17, 2024

Extra thought: maybe we should always have an abortion button, also if govmetrics isn't enabled 🤔 Then we could refactor ExistingSubmissionOptions further

@SilviaAmAm SilviaAmAm force-pushed the feature/180-govmetric-2 branch from 8740682 to e6bbcc6 Compare January 17, 2024 12:15
@SilviaAmAm SilviaAmAm requested a review from Viicos January 18, 2024 09:55
@Viicos
Copy link
Contributor

Viicos commented Jan 18, 2024

Extra thought: maybe we should always have an abortion button, also if govmetrics isn't enabled 🤔 Then we could refactor ExistingSubmissionOptions further

I think it could be great, as a user I would expect an explicit way to abort, without having to close the browser and wonder about what actually happens when I do so

@SilviaAmAm
Copy link
Contributor Author

Follow up ticket: open-formulieren/open-forms#3791

src/api-mocks/submissions.js Outdated Show resolved Hide resolved
src/components/AbortionButton/AbortionButton.mdx Outdated Show resolved Hide resolved
src/components/AbortionButton/AbortionButton.mdx Outdated Show resolved Hide resolved
src/components/AbortionButton/AbortionButton.stories.js Outdated Show resolved Hide resolved
src/story-utils/decorators.js Outdated Show resolved Hide resolved
src/components/App.stories.js Outdated Show resolved Hide resolved
@sergei-maertens
Copy link
Member

Please see the comments on the Chromatic build - there's some empty blank space now that should ideally not be rendered if the abortion button is not relevant.

@SilviaAmAm SilviaAmAm force-pushed the feature/180-govmetric-2 branch from 39d2f14 to d8d8e81 Compare January 29, 2024 12:01
@SilviaAmAm SilviaAmAm marked this pull request as draft January 29, 2024 13:01
@SilviaAmAm SilviaAmAm force-pushed the feature/180-govmetric-2 branch from d8d8e81 to 8845f0b Compare January 29, 2024 13:06
@SilviaAmAm SilviaAmAm marked this pull request as ready for review January 29, 2024 13:07
@sergei-maertens sergei-maertens merged commit da91b7e into main Jan 30, 2024
15 of 19 checks passed
@sergei-maertens sergei-maertens deleted the feature/180-govmetric-2 branch January 30, 2024 11:41
@sergei-maertens
Copy link
Member

Also merged the main branch in stable/2.5.x 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
3 participants