Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[Feat] Add employee profile page #12371
Changes from 6 commits
bdebdff
3efec5b
bb06028
24895da
8d5cd48
2af1dcd
9268040
aca9b09
d9e6321
85e2a30
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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:
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.
Oh, the hero does go centred on its own. I didn't test narrow enough.
All the admin page refreshes use centred headings in mobile widths. I don't know if this is standard enough to include in the base component or not.
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
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.
@esizer Does this function satisfy the acceptance criteria for #12363 and then allow it to be closed as well?
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.