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

Add component to show submission review state #1071

Merged
merged 4 commits into from
Dec 8, 2024

Conversation

matthew-white
Copy link
Member

@matthew-white matthew-white commented Dec 4, 2024

Different components show a submission review state. To do so, they use the reviewStateIcon() function of the useReviewState() composable along with the reviewState.* messages in en.json5. However, one piece of functionality that isn't shared is the review state color: each component that shows a review state manages the color in its own way. This PR changes that by introducing a new component named SubmissionReviewState. It will show the icon and message associated with the review state, and it will also add the review state color.

I'm making this change with getodk/central#670 in mind. The hover card for a submission will show the submission's review state, and I didn't want to copy the review state colors to yet another component.

What has been done to verify that this works as intended?

I updated tests. I also checked locally in the browser most of the components where I introduced SubmissionReviewState. If any issues are found during regression testing, we can patch those.

Why is this the best possible solution? Were any other approaches considered?

I think a component is exactly the right thing for the job, and I'm not sure why I didn't add one sooner. I may have been avoiding adding a component with SubmissionMetadataRow in mind. I've been trying to make that component as fast as possible to render, since there can be many submission rows on the page, and I've observed in the past that child components carry a performance penalty. However, in v2025.1, we'll be adding pagination to the submissions table, which should alleviate performance concerns. I've noticed before that one surprising performance bottleneck in components is Vue I18n. However, the new component doesn't have its own i18n messages — it only uses global messages from en.json5 — which should help.

Before submitting this PR, please make sure you have:

  • run npm run test and npm run lint and confirmed all checks still pass OR confirm CircleCI build passes
  • verified that any code or assets from external sources are properly credited in comments or that everything is internally sourced

@matthew-white matthew-white requested a review from ktuite December 4, 2024 23:42
Copy link
Member Author

@matthew-white matthew-white left a comment

Choose a reason for hiding this comment

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

Notes from interactive review

src/components/submission/metadata-row.vue Outdated Show resolved Hide resolved
src/components/submission/metadata-row.vue Show resolved Hide resolved
@matthew-white
Copy link
Member Author

I've pushed a commit with some changes. The biggest change is that I've removed some additional tests, similar to changes to other tests that @ktuite and I reviewed. I don't think we necessarily need another round of code review for this commit.

@matthew-white matthew-white merged commit c259f04 into master Dec 8, 2024
1 check passed
@matthew-white matthew-white deleted the review-state-component branch December 8, 2024 20:55
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