-
Notifications
You must be signed in to change notification settings - Fork 3
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
Fix/tpi/home page hero styles #264
Conversation
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 found couple of mismatches with the design, check my comments ☝️
app/assets/stylesheets/tpi.scss
Outdated
@@ -40,3 +40,9 @@ | |||
html { | |||
scroll-behavior: smooth; | |||
} | |||
|
|||
body { | |||
@include mobile { |
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 our approach was to have default settings for mobile and then changing it accordingly for desktop 🤔
so, it would be:
body {
overflow-x: hidden;
@include desktop { ... styles for desktop... }
}
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.
Will do 👍
1: I'm using the lowest quality one there, yeah, maybe the 2x should be better as a default, although any Both scenarios require svg handling. I can go for it but I can't predict the outcome nor the time it will take. Everything else, damn! That's some sharp eye right there! |
Hey @batraisk do you mind picking this PR up, update it with the base branch and address Marta's comments. You might need to ask @faustoperez for some assets that Rui said were missing. If it requires svg handling let's not do it for now. Just the simplest changes! Thanks! |
@simaob , Of course, I’ll do it now) |
@simaob , I've finished. check please) |
fixed |
No description provided.