-
Notifications
You must be signed in to change notification settings - Fork 9
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weβll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Feat] Add employee profile page #12371
Conversation
isWorkEmailVerified?: Maybe<boolean>; | ||
} | ||
|
||
export const isVerifiedGovEmployee = ({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May need to wait and see. I think to but not 100% sure
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this does satisfy 12363.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isWorkEmailVerified
is computed server-side making it not 100% frontend but I'm not sure if that is a problem. Issue linked has no A/C or details.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good! A few things to check on, though. I've left questions for Josh and Nienke, who will need to approve before merging, too.
return ( | ||
<> | ||
<SEO title={pageTitle} description={subtitle} /> | ||
<Hero title={pageTitle} subtitle={subtitle} crumbs={crumbs} /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In mobile widths, the Hero should be centred. The section headers should be, too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, this is just using our base hero/heading components. That would require modifying it specifically for this page which seems odd. I feel like we should either:
- Update the base component so we have consistent styles on all pages
- Or leave this page alone so we don't have special styles for one page
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I vote: leave this page alone so we don't have special styles for one page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
data-h2-gap="base(x3 0)" | ||
> | ||
<TableOfContents.Section id={SECTION_ID.CAREER_PLANNING}> | ||
<Heading |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to Figma, this should be font-weight 400.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I can do that but this is also using the base heading component and we should avoid changing font styles for headings because they need to be consistent for visual users to indicate content hierarchy for accessibility.
Should the heading 2 have 400? If not, I don;'t think we should be messing with font weight since it can gave a negative affect on the sites accessibility.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, the designers have explicitly said there is no standard font weight for a given heading level and should be set on a case-by-case basis.
#11859 (comment)
You could check in with a designer and see if the design could be changed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For both heading related things:
h1
,h2
,h3
at a minimum should always be centered on mobile devicesh2
s will generally be400
weight in most cases if you need to set a default on the component - this has been the case for a while in the designs and it's fine to propagate it everywhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we are editing the base component, would it be better to create a separate ticket to fix our base heading? It seems a little out of scope for a basic stub page to affect all other pages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I'll write one up. π
#11859
const pageTitle = intl.formatMessage({ | ||
defaultMessage: "Your empoloyee profile", | ||
id: "HwW7BZ", | ||
description: "Page title for a users employee profile", | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your employee profile*
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good eye, fixed in update copy
We'd like to make a change to the copy. Instead of "talent mobility preferences", we want to use "career development preferences".
|
Yup! We have done that as part of update copy |
I think it's just the header font-weight and centering on mobile left. |
Sorry, I thought we planned on doing that as a separate ticket to fix the underlying components instead of doing single page overrides (something we should be avoiding). |
Let's make a decision in standup today. |
okay, based on decision during stand up, I have added in the overrides: override heading |
π€ Resolves #12002
π Introduction
Adds the employee profile page with table of contents and initial section.
π΅οΈ Details
I believe the check for government employee makes sense π€ All must be true:
isGovEmployee
workEmail
workEmailisVerified
π§ͺ Testing
pnpm run dev:fresh
/applicant/employee-profile
πΈ Screenshot