Adding share terminology to the My Grants view #3160
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
github-actions
bot
added
the
javascript
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: @mayabose, Action: |
jeffsmohan
suggested changes
Jun 11, 2024
packages/client/src/router/index.js
Outdated
| { | ||
| path: '/my-grants/assigned', | ||
| name: 'assigned', | ||
| redirect: shareTerminologyEnabled() ? '/my-grants/shared-with-your-team' : undefined, |
There was a problem hiding this comment.
It's a bit more DRY to use the named route here — { name: 'shared-with-your-team' } — instead of the raw route string
packages/client/src/router/index.js
Outdated
| @@ -92,7 +92,7 @@ export const routes = [ | |||
| name: 'myGrants', | |||
| component: () => import('../views/MyGrantsView.vue'), | |||
| meta: { | |||
| tabNames: ['interested', 'assigned', 'not-applying', 'applied'], | |||
| tabNames: [shareTerminologyEnabled() ? 'shared-with-your-team' : 'assigned', 'interested', 'not-applying', 'applied'], | |||
There was a problem hiding this comment.
I'm seeing two minor issues here that I think we could improve with a small refactor. The issues:
- Now that we're including a ternary in the
tabNameslist here, it's feeling a bit more complex than would be ideal to inline in the meta definition here, as it's getting a bit hard to read. - We really want the
/my-grantsroute above to redirect to the first tab (as opposed to explicitly the interested tab.
I think we can refactor this to solve both issues pretty neatly by determining the list of tab names elsewhere, like at the top of the file here. (Ideally, we might someday split up route definitions into multiple files and then we could keep this closer to the routes themselves. But for now, top of this file feels fine.) So...
const myGrantsTabs = [
shareTerminologyEnabled() ? 'shared-with-your-team' : 'assigned',
'interested',
'not-applying',
'applied',
];And then the /my-grants route can redirect based on the first tab: redirect: { name: 'myGrants', params: { tab: myGrantsTabs[0] } }. And the tabNames here can simply reference myGrantsTabs.
| @@ -7,20 +7,27 @@ | |||
| style="margin-top: 1.5rem" | |||
| lazy | |||
| > | |||
| <b-tab title="Interested"> | |||
| <b-tab v-if="shareTerminologyEnabled" title="Shared With Your Team"> | |||
There was a problem hiding this comment.
Have you set up linting and auto-formatting in your editor yet? You should hopefully be able to get running with auto-fixing for lint errors like what's showing up on this and the line below.
Bit of documentation is here, but I'm happy to help as well if you need! https://github.com/usdigitalresponse/usdr-gost/blob/main/docs/development.md#linting
There was a problem hiding this comment.
That aside, given that the functionality doesn't really change, but only the tab title, it's a bit more DRY here to just make the titles dynamic:
<b-tab :title="shareTerminologyEnabled ? 'Shared With Your Team' : 'Assigned'">
<GrantsTable
:search-title="shareTerminologyEnabled ? 'Shared With Your Team' : 'Assigned'"
:show-assigned-to-agency="selectedAgencyId"
:show-search-controls="false"
/>
</b-tab>You could also turn the shareTerminologyEnabled ? 'Shared With Your Team' : 'Assigned' into a computed property if you want, but one repetition I'm on the fence about whether it's worth it.
There was a problem hiding this comment.
Thank you for the detailed response and code suggestions!
jeffsmohan
suggested changes
Jun 13, 2024
packages/client/src/router/index.js
Outdated
| @@ -103,6 +110,23 @@ export const routes = [ | |||
| } | |||
| }, | |||
| }, | |||
| { | |||
There was a problem hiding this comment.
Sorry, I missed this in my first review, but these new route definitions are never actually being hit. vue-router (in Vue 2, at least) evaluates a URL against the routes in order and uses the first matching route. So the above /my-grants/:tab always matches before getting to these more-specific routes.
If we want the gating and redirecting defined below to work, you'll need to move these two new route definitions above the /my-grants/:tab route.
(Interesting note: If we migrate to Vue 3 soon, that version of vue-router actually has a path ranking algorithm that would have identified your new routes as more specific than the generic tab route, and would have prioritized them regardless of the order. Example evaluation here.)
packages/client/src/router/index.js
Outdated
| { | ||
| path: '/my-grants/assigned', | ||
| name: 'assigned', | ||
| redirect: { name: 'shared-with-your-team' }, |
There was a problem hiding this comment.
I think in the last set of changes, you lost the flag check here. We want to only redirect if the new terminology flag is on.
There was a problem hiding this comment.
This might be a good justification for writing a test for the changes made to this route :)
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 #3061
Description
Implements the following changes to the
My Grantspage:Assignedtab is updated toShared With Your Team/my-grants/assignedis updated to/my-grants/shared-with-your-team. The Vue router also redirects from/assignedto/shared-with-your-team.Shared With Your Team/Assignedis first.All of these changes (except for the order of the tabs) is dependent on the state of the
shareTerminologyEnabledfeature flag.Testing
The tab, title, and url of the
Assignedtab is correctly updated toShared With Your Teamwhen the feature flag is set totrue, and remains unchanged when the flag is set tofalse.Checklist