WIP: feat: Conditionally filter the budget enrollments #1028
Conversation
zamanafzal
force-pushed
the
zafzal/ENT7446
branch
from
436dffd
to
a99e32b
Compare
| <Button | ||
| data-testid="view-budget" | ||
| onClick={() => navigateToBudgetRedemptions(budgetType)} | ||
| onClick={() => navigateToBudgetRedemptions(id)} |
There was a problem hiding this comment.
[suggestion] I would recommend treating this Button as a browser hyperlink with the react-router's Link component rather than relying on a JavaScript-only history.push. Users might want to open the budget details page in a new tab (e.g., right click and "Open in new tab", or Cmd + click on the button).
<Button
data-testid="view-budget"
as={Link}
to={`/${enterpriseSlug}/admin/${ROUTE_NAMES.learnerCredit}/${id}`}
>
View budget
</Button>| @@ -105,6 +106,11 @@ const EnterpriseAppRoutes = ({ | |||
| /> | |||
| )} | |||
|
|
|||
| <Route | |||
| exact | |||
| path={`${baseUrl}/admin/${ROUTE_NAMES.learnerCredit}/:id`} | |||
There was a problem hiding this comment.
nit: perhaps :budgetId to be slightly more explicit?
| @@ -105,6 +106,11 @@ const EnterpriseAppRoutes = ({ | |||
| /> | |||
| )} | |||
|
|
|||
| <Route | |||
There was a problem hiding this comment.
[suggestion] Given this Route and the above Route both relate to the same Learner Credit Management (LCM) feature, I'm wondering if this component should be defining a single parent route for the overall LCM feature, and then the component passed to the parent route has its owned rendered sub-routes for the LCM-specific routes. That way, EnterpriseAppRoutes, doesn't really need any knowledge of the detail route.
Related, as is, this LCM detail page route would be available even when the LCM overview page route (defined above) is not since it isn't relying on the canManageLearnerCredit variable. If the overview page route isn't available; neither should the detail page route.
For all changes
Only if submitting a visual change