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
[UI] Welcome modal update #1163
[UI] Welcome modal update #1163
Changes from 5 commits
8f0687c
09e3b6c
b5cfaec
4022ba7
155aa16
884693d
64e0a58
27740b2
fa6e13b
68452be
ee9e966
d69bb64
dcd6504
c5739ef
2fad827
0aa1d6f
281de19
6d41c94
b2515a0
8c963ea
923bd48
060b839
925956d
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.
Do we need this? It seems odd to me that we would set the font size instead of using a typography style, especially on hover
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.
This was already in the file, I only changed it from lg to md;
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.
We pretty much never have to set the specific typography in a component since we can reuse the styles from hp/ui
src\styles\designSystem\_typography.scss
. Do we need a new style or to update an existing style in the global stylesheet? If we make custom type sizing, weights, fonts, or line-heights on a per component basis, the typography system quickly become unmaintainable.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 had to create a class to style a small piece of text differently because it was inheriting bold from its parent. I'll change it and use var instead the next time.
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.
this class doesn't seem to be used anywhere
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.
should this be a translation string?
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 this one, can you not use a number as a key, this might give issues in the future.
Rename it to something like
intro
for instance.