feat/my applications #1079
feat/my applications #1079
Conversation
emilyjablonski
added
the
not ready yet
|
Deploy preview for clever-edison-cd22c1 ready! Built with commit ad94b00 https://deploy-preview-1079--clever-edison-cd22c1.netlify.app |
emilyjablonski
added
ready for review
and removed
not ready yet
| /> | ||
| ))} | ||
| {applications.length == 0 && noApplicationsSection} | ||
| {applications && |
There was a problem hiding this comment.
We could probably use optionals here to condense it a bit?
| interface AppStatusItemProps { | ||
| application: Application | ||
| listing: Listing | ||
| status: string | ||
| setDeletingApplication: (application: Application) => void | ||
| lotteryNumber?: string |
There was a problem hiding this comment.
I'm not familiar with the changes to remove this AppStatusItem functionality. Can you let me know why this changed?
There was a problem hiding this comment.
Sure! So the designs I had didn't have anything in them to delete applications so I removed that piece, and then there was an older TODO on line 9 "// TODO status and lotteryNumber should be loaded from Application"
|
@emilyjablonski Regarding your questions: (1) seems good to me… (2) it'd be prudent to add a security check before merging in this PR. |
|
Gotcha, I think we'll need to have a conversation as a full team about how to approach those kinds of security checks across the app before we can move forward with this then 👍 |
emilyjablonski
force-pushed
the
feat/my-applications
branch
from
bb39cb6
to
bc93fdd
Compare
|
There's some better context from Michal in this issue, but the security concerns are not a problem here :) The backend will not retrieve an application that belongs to another user (I tested it out locally to be sure). I will need some copy in the "don't have access" case - right now it just renders a blank page - I requested it from Jesse and then can update this PR w that info |
emilyjablonski
force-pushed
the
feat/my-applications
branch
from
70622a3
to
99e98d5
Compare
|
This is ready again! |
|
@emilyjablonski Updated some simple text styles. Otherwise LGTM |
|
@jaredcwhite This is the one we were just chatting about :) |
|
I'll take a look at this first thing in the morning! |
| import { useRouter } from "next/router" | ||
| import moment from "moment" | ||
|
|
||
| const dateSubmitted = (submissionDate: Date) => { |
There was a problem hiding this comment.
This seems like it could be a useful "generic" helper. Maybe we can relocate to ui-components?
There was a problem hiding this comment.
Re-located :)
| const [application, setApplication] = useState<Application>() | ||
| const [listing, setListing] = useState<Listing>() | ||
| const [unauthorized, setUnauthorized] = useState(false) | ||
| const [noApplication, setNoApplication] = useState(false) |
There was a problem hiding this comment.
These 403/404 state hooks are neat. I wonder if we could make a standard hook that would provide this and set the appropriate state on either 404 or 403, so we could use it on other pages. This might be good to review as well if we move towards using SWR everywhere: https://swr.vercel.app/docs/error-handling
There was a problem hiding this comment.
Refactored this to be a little cleaner, but would hope we could accomplish this in another ticket?
| .then((apps) => { | ||
| setApplications(apps) | ||
| }) | ||
| .catch((err) => console.error(`Error fetching applications: ${err}`)) |
There was a problem hiding this comment.
On this page it looks like any error condition will simply show that no applications are present. Could be frustrating if somebody's seeing that instead of at least a message that some kind of error occurred.
There was a problem hiding this comment.
Made this change, thanks!
…ng/bloom into feat/my-applications
This PR adds the ability for a signed-in user to view their submitted applications.
You can go to your account page and then click on My Applications to access it!