feat-317: Add profile menu for applicants #330
Conversation
| useEffect(() => { | ||
| if (value !== undefined) { | ||
| setSelectedValue(value); | ||
| } | ||
| },[value, setSelectedValue]) | ||
|
|
There was a problem hiding this comment.
is this needed?
I think this line further down does more or less the same:
value={selectedValue || ""}
Oh! Wait. I see, you added a value prop that we receive now. But I don't see where you are using that new prop.
The thing is that before what we were doing was that forms using this component are usually ValidatedForms. Which can receive a defaultValues prop that has an object with properties and values. This line further up gets the value for this component from that defaultValues property by it's name:
const [selectedValue, setSelectedValue] = useControlField<string>(name);
app/core/layouts/Header.tsx
Outdated
| export const loader: LoaderFunction = async ({ request }) => { | ||
| try { | ||
| const profile = await requireProfile(request); | ||
| const applicant = await getApplicantByEmail(profile.email); | ||
|
|
||
| return { | ||
| applicant, | ||
| }; | ||
| } catch (error) { | ||
| console.error("Error loading data:", error); | ||
| } | ||
| }; | ||
|
|
There was a problem hiding this comment.
This is not a route component, so I think we don't know if the loader will work or not. Or rather, we don't know what route is going to use this component, many routes will, so we are not guaranteed to have this function ok.
Regular components (outside a Route component) should not use loaders, and instead just require any values as props.
ruth-avelar
force-pushed
the
feat-317-Allow-applicants-to-edit-profile
branch
from
4115455 to
98ed7ad
Compare
jackbravo
force-pushed
the
feat-317-Allow-applicants-to-edit-profile
branch
from
79392ce to
a007f8e
Compare
app/core/layouts/Header.tsx
Outdated
| existApplicant?: boolean; | ||
| applicantId?: string | number; |
There was a problem hiding this comment.
Maybe we can just have applicantId, if we allow undefined we can check if its set or not, instead of using existApplicant
app/core/layouts/Header.tsx
Outdated
| const handleClickProfileApplicant = async () => { | ||
| submit(null, { | ||
| method: "get", | ||
| action: `/applicants/${applicantId}`, | ||
| }); | ||
| }; | ||
|
|
||
| const handleLogout = async () => { |
There was a problem hiding this comment.
Oh! I haven't reviewed this code when it was first added (I mean for example the other handleClickProfile function). In general, I think it is better to use proper links instead of onClick actions and functions that depend on react. Links already provide good functionality, you can right-click on them to open in new tab, etc.
| @@ -27,11 +28,13 @@ const StyledHeaderButton = styled(Button)(({ theme }) => ({ | |||
| }, | |||
| })); | |||
|
|
|||
| const Header = ({ existApplicant }: IProps) => { | |||
|
|
|||
| const Header = ({ existApplicant, applicantId }: IProps) => { | |||
| const currentUser = useOptionalUser(); | |||
There was a problem hiding this comment.
I wonder if we could add a useOptionalApplicant() function, to not have to pass the applicantId as a parameter... 🤔
What does this PR do?
Add the profile menu for the applicant
When click 'Profile' menu it goes to applicant/$id
Then there is an edit icon and it goes to applicationForm and the form can be editable
Where should the reviewer start?
How should this be manually tested?
Any background context you want to provide?
What are the relevant tickets?
Screenshots
Questions
Checklist