-
Notifications
You must be signed in to change notification settings - Fork 34
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
Conversation
All contributors have signed the CLA ✍️ ✅ |
I have read the CLA Document and I hereby sign the CLA |
<div> | ||
{t( | ||
'hyperplay.onboarding.welcome.text.communityCTA_1', | ||
`We'd love your feedback and to have you join us in our` | ||
'hyperplay.onboarding.welcome.text.2', |
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.
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.
Pretty good stuff! 💪🏽
Code overall looks good, just left some comments about the i18n keys.
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.
ui is looking good. let's take the time to make it responsive since this is the first impression on new users
@@ -32,8 +39,38 @@ | |||
.welcomeBodyTextContainer a:hover { | |||
color: var(--color-primary-300); | |||
transition: 0.2s; | |||
font-size: var(--font-size-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.
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;
.diamondBlue { | ||
width: 10px; | ||
height: 10px; | ||
margin-right: 8px; |
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.
margin-right: 8px; | |
margin-right: var(--space-xs-fixed); |
.text { | ||
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.
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.
align-items: flex-start; | ||
} | ||
|
||
.scrollableModal { |
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
) | ||
} | ||
> | ||
Compatibility Layer (Beta) |
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?
I didn't change anything in the responsiveness of this component I just changed the text and changed the position of the language selector and the button. |
* update welcome modal to be responsive * update modal padding
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.
LGTM
@@ -34,6 +41,29 @@ | |||
transition: 0.2s; | |||
} | |||
|
|||
.boldText { | |||
.text { | |||
font-weight: var(--body-sm); |
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 don't think --body-sm
is defined 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.
<--- Update Welcome Modal --->
Updated the copy and some styles of the Welcome to HyperPlay modal
Design to compare here
Use the following Checklist if you have changed something on the Backend or Frontend: