[To Main] Add tenant listing page #2503
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2503 +/- ##
==========================================
+ Coverage 75.78% 75.81% +0.03%
==========================================
Files 590 597 +7
Lines 21133 21363 +230
Branches 1529 1568 +39
==========================================
+ Hits 16016 16197 +181
- Misses 4870 4912 +42
- Partials 247 254 +7
Flags with carried forward coverage won't be shown. Click here to find out more.
|
There was a problem hiding this comment.
Looks great! Could you please:
- add the ticket reference to the Changelog entry
- remove the console.log
- review the naming of "Feedback" in resources/tenant.py
- review if the material UI icon can be used with fontawesome (or let me know if it's not possible)
I'd also love to just chat briefly about some of the infrastructure decisions you made with the new design system components. It would be great if all the developers were on the same page with how we're going to start integrating Jess' design system :)
CHANGELOG.MD
Outdated
| @@ -1,3 +1,10 @@ | |||
| ## May 9, 2024 | |||
|
|
|||
| - **Feature** Added the initial version of the tenant listing page | |||
There was a problem hiding this comment.
Don't forget a reference to the ticket!
|
|
||
| return tenants, HTTPStatus.OK | ||
| except ValueError as err: | ||
| return str(err), HTTPStatus.INTERNAL_SERVER_ERROR | ||
|
|
||
| @cors_preflight('GET OPTIONS') | ||
| @API.route('/<tenant_id>') | ||
| class Feedback(Resource): |
There was a problem hiding this comment.
I'm guessing this needs to be:
| class Feedback(Resource): | |
| class Tenant(Resource): |
| @@ -0,0 +1,89 @@ | |||
| import React from 'react'; | |||
There was a problem hiding this comment.
I appreciate that you've started to add some high fidelity, reusable components based on Jess' work! Just curious about how you chose to organize it:
- The intent was to replace our app-wide buttons, right? Is there a reason we have this and we're keeping the other button types? See: https://github.com/bcgov/met-public/blob/b3921c6bd8191859546dcc6fd5849a78d013723c/met-web/src/components/common/index.tsx
- Is there a reason you made a dedicated folder called
MetDesignSystem? Aren't these just supposed to be the default buttons we use app-wide? The naming would make sense to me if we were maybe exposing an external API of our common components, but my understanding is that the MET design system is just for us.
| smallScreenOnly, | ||
| }) => { | ||
| return ( | ||
| <Breadcrumbs aria-label="breadcrumb" sx={smallScreenOnly ? { display: { xs: 'block', md: 'none' } } : {}}> |
There was a problem hiding this comment.
I feel like it'd be a bit more Reactive to have screen size check happen outside the component and have it not render, rather than having it built in and trigger a display:none. Although I get the impulse to have a component be super self-contained.
| outline: `2px solid ${colors.focus.regular.outer}`, | ||
| boxShadow: globalFocusShadow, | ||
| }, | ||
| }; |
There was a problem hiding this comment.
I appreciate that you took the trouble to get all these styling variables accounted for - and for creating those new design system components.
MET already has lots of common components in use. Just curious if you looked into how much of it we can massage into the Design System rather than creating everything from scratch.
| @@ -0,0 +1,114 @@ | |||
| import { AddCircleOutline } from '@mui/icons-material'; | |||
There was a problem hiding this comment.
Should this be with fontawesome?
There was a problem hiding this comment.
Yes; I was waiting for the Font Awesome PR to be merged
|
|
||
| useEffect(() => { | ||
| const fetchTenants = async () => { | ||
| const tenants = await getAllTenants(); |
There was a problem hiding this comment.
You might be able to save a couple lines of code(plus add more specific error handling), and the function declaration with:
| const tenants = await getAllTenants(); | |
| getAllTenants | |
| .then(returnedTenants => setTenants(returnedTenants) | |
| .catch(// Handle Error) | |
| .finally(() => setLoading(false)) |
There was a problem hiding this comment.
Unfortunately, .finally() isn't available because we're compiling in ES2017 mode
There was a problem hiding this comment.
I believe you should be able to use it if you want to still. I think the target option just indicates which ES features are "downgraded" and which are left intact.
|
Issue #: https://apps.itsm.gov.bc.ca/jira/browse/DESENG-592
Description of changes:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of the met-public license (Apache 2.0).