Assign-to-share: Update grant detail page #3208
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ucdtran/assign-to-share-update-grant-detail-page
…ucdtran/assign-to-share-update-grant-detail-page
github-actions
bot
added
the
javascript
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
QA Summary
Test CoverageCoverage report for `packages/client`
Coverage report for `packages/server`
|
Terraform Summary
Hint: If "Terraform Format & Style" failed, run OutputValidation OutputPlan Summary
Pusher: @thucdtran, Action: |
jeffsmohan
suggested changes
Jun 24, 2024
| <p class="m-0 text-muted"> | ||
| <small>{{ formatDateTime(agency.created_at) }}</small> | ||
| </p> | ||
| <div v-if="!shareTerminologyEnabled"> |
There was a problem hiding this comment.
Two small tweaks here:
- The two
v-ifstatements are slightly more readable/robust if we use av-iffollowed by av-else. (e.g., if the condition got changed at some point, we wouldn't need to change it in two locations) - In a situation like this where we don't actually need or want a
divtag in our html structure, we can use vue'stemplatetag to avoid polluting the document structure.
| @@ -378,6 +401,9 @@ export default { | |||
| newTerminologyEnabled() { | |||
| return newTerminologyEnabled(); | |||
| }, | |||
| shareTerminologyEnabled() { | |||
| return shareTerminologyEnabled(); | |||
| }, | |||
There was a problem hiding this comment.
(very minor/optional/non-blocking) This can be simplified to shareTerminologyEnabled, which is a bit more terse and makes it exceedingly clear that we're not wrapping the helper method to change its functionality in any way.
There was a problem hiding this comment.
Not quite sure, what you mean here; does this mean to change this to appear like:
newTerminologyEnabled() {
return newTerminologyEnabled();
},
shareTerminologyEnabled,
?
There was a problem hiding this comment.
Ah, sorry, I think the suggestion became so terse that it lost its context and was a bit tough to understand. To be more clear, you can apply this diff and the behavior will be identical:
diff --git a/packages/client/src/views/GrantDetailsView.vue b/packages/client/src/views/GrantDetailsView.vue
index afc16fbb..623178c9 100644
--- a/packages/client/src/views/GrantDetailsView.vue
+++ b/packages/client/src/views/GrantDetailsView.vue
@@ -404,12 +404,8 @@ export default {
(interested) => interested.agency_id.toString() === this.selectedAgencyId,
);
},
- newTerminologyEnabled() {
- return newTerminologyEnabled();
- },
- shareTerminologyEnabled() {
- return shareTerminologyEnabled();
- },
+ newTerminologyEnabled,
+ shareTerminologyEnabled,
unassignedAgencies() {
return this.agencies.filter(
(agency) => !this.assignedAgencies.map((assigned) => assigned.id).includes(agency.id),Again, totally optional and non-blocking!
…ucdtran/assign-to-share-update-grant-detail-page
jeffsmohan
approved these changes
Jul 1, 2024
…ucdtran/assign-to-share-update-grant-detail-page
jeffsmohan
approved these changes
Jul 2, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Ticket #3071
Description
This ticket implements the UI changes specified by the update grant detail issue.
Screenshots / Demo Video