Skip to content

Optimised first pages on mobile #388

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

Merged
merged 3 commits into from
Jul 5, 2018
Merged

Conversation

mllocs
Copy link
Collaborator

@mllocs mllocs commented Jun 23, 2018

Use a smaller background images on mobile devices and optimised the current one.

  • On desktop we went from 1,9MB to 1MB
  • On mobile from 1,9MB to 333KB

Other minor details to improve the look of the "first pages" (Home and Login).

@mllocs
Copy link
Collaborator Author

mllocs commented Jun 23, 2018

@markets
Copy link
Collaborator

markets commented Jun 25, 2018

good job @mllocs! 👍 do you think this branch overlaps with #385 (a fix for the classic #278)? I think so, maybe we can try to coordinate with @Matt-Yorkley in order to port/integrate his changes (if needed) here? What do you think guys?

@mllocs
Copy link
Collaborator Author

mllocs commented Jun 26, 2018

Hey @markets, yes, it totally overlaps with #385, good catch.

Here I'm also fixing other issues, sorry for mixing it up..., here I'm getting rid of .vertical-align because it was only used in two places, and they didn't have much in common. @Matt-Yorkley can you take a look? 😄

@@ -63,6 +63,7 @@
& {
background-color: transparent;
border: 1px solid white;
padding: 4px 8px
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔚 ;

Copy link
Collaborator

@markets markets left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mllocs minor comment added, but 👍

@Matt-Yorkley
Copy link
Contributor

@mllocs Ok, so my other PR for the login modal might not be needed then. I notice you've used top: 20% for the desktop view, and top: 60px for the small/mobile view. Is it worth putting it at a fixed position on both, so that it's position is always the same relative to the navigation bar? Like just having top: 60px for both screen sizes?

@enricostano
Copy link
Contributor

enricostano commented Jul 3, 2018

Deployed on staging, not sure about performance improvement but I found the following zoomed background image on mobile:

screenshot_20180703-113514

Is that normal @mllocs ?

EDIT: in develop is different, much less zoomed in.

EDIT2: actually I see a small performance boost

@mllocs
Copy link
Collaborator Author

mllocs commented Jul 4, 2018

@enricostano good catch, fixed ;)

@Matt-Yorkley on your suggestion, I tried to use the same percentage in the top attribute for both big screens and mobile but it didn't look well, on big screens there was too much empty space :)

@enricostano
Copy link
Contributor

Tested on staging 🍏

@enricostano enricostano merged commit 617c724 into develop Jul 5, 2018
@enricostano enricostano mentioned this pull request Aug 8, 2018
@markets markets deleted the fix/first-pages-on-mobile branch February 16, 2019 13:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants