Skip to content
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

#2086 Add Lucha Libre Mask #2121

Merged
merged 20 commits into from
Jun 6, 2020
Merged

Conversation

MattJ12
Copy link
Collaborator

@MattJ12 MattJ12 commented May 31, 2020

Resolves #2086

This PR allows the site to switch to the Lucho Libre Mask Logo for the correct cities or keep the regular site logo. I changed the logo on the mobile validation interface in the same way, but I was not able to test this on a physical device. I was able to see through the Chrome developer console using their simulated mobile devices that the Lucha Libre logo is formatted correctly on mobile.

Regular Logo:
sidewalk-logo-new

Lucho Libre Logo:
sidewalk-logo-new

@MattJ12 MattJ12 changed the title #2086 Ddd Lucha Libre Mask #2086 Add Lucha Libre Mask May 31, 2020
@misaugstad
Copy link
Member

Is it not possible to get the city string directly from the template Scala? :( This is fine if we have to do it, it just seems like overkill. And ideally we'd be able to get that info directly in the navbar instead of having to pass it twice for every webpage.

@MattJ12
Copy link
Collaborator Author

MattJ12 commented May 31, 2020

Is it not possible to get the city string directly from the template Scala? :( This is fine if we have to do it, it just seems like overkill. And ideally we'd be able to get that info directly in the navbar instead of having to pass it twice for every webpage.

I based my changes off of what was done in the index.scala.html file to get the correct skyline. To get the correct skyline, the parameter cityId is passed into index.scala.html from a controller. This is why I passed the cityId variable for the rest of the template files. I believe that the city string can be accessed from a Scala template file like what was done in navbar.scala.html at line 91. The issue is that I don't think passing that value of the city string obtained in the Scala template into the "@routes.Assets.at("assets/" + cityId + "/sidewalk-logo-new.png")" line of code in navbar.scala.html works. I tried different ways of passing the city string obtained from the Scala template file into the above line and I couldn't get it to work.

@misaugstad
Copy link
Member

Thanks for pointing out line 91 of navbar.scala.html! So it looks like you should definitely be able to do this just in there. Could you try troubleshooting that approach a bit more?

@misaugstad
Copy link
Member

And if you aren't able to figure it out for awhile, can you show me the code you're trying and the issue you're having?

@MattJ12
Copy link
Collaborator Author

MattJ12 commented May 31, 2020

Thanks for pointing out line 91 of navbar.scala.html! So it looks like you should definitely be able to do this just in there. Could you try troubleshooting that approach a bit more?

I was able to access the city string in the navbar Scala template file, a much cleaner solution. I also changed the mobile validation logo to depend on the user's city, but I was not able to test this because my mobile dev environment is broken.

@MattJ12
Copy link
Collaborator Author

MattJ12 commented Jun 2, 2020

I just updated the details and size of the logo, does it look good?

@misaugstad
Copy link
Member

It looks like it still doesn't have a transparent background and looks like it could be cropped a bit more?

@MattJ12
Copy link
Collaborator Author

MattJ12 commented Jun 3, 2020

It looks like it still doesn't have a transparent background and looks like it could be cropped a bit more?

I made the background transparent and cropped it a little bit more.

@misaugstad
Copy link
Member

Thanks. Looks like there is still more cropping for it to line up with the current logo. Here is a zoom in pic of the current logo followed by the current state of the lucha libre one:
correct-crop
more-to-crop

And thanks for making it more compressed! Could I actually get a sliiightly less compressed one to get a tiny bit more detail?

@MattJ12
Copy link
Collaborator Author

MattJ12 commented Jun 3, 2020

Thanks. Looks like there is still more cropping for it to line up with the current logo. Here is a zoom in pic of the current logo followed by the current state of the lucha libre one:
correct-crop
more-to-crop

And thanks for making it more compressed! Could I actually get a sliiightly less compressed one to get a tiny bit more detail?

Done!

@misaugstad
Copy link
Member

Okay, so on mobile the mask doesn't look any good unless we improve the resolution. I played around with it and got a resolution I'm happy with. It's like 45 kb instead of 15 kb, but I think that's fine. Worth it to have a crisp logo.

I'm attaching it here. Could you add it for the two Mexican cities, and then also create an edited version for the other cities that is at that resolution?

sidewalk-logo

@misaugstad
Copy link
Member

Oh and here's a high res one. Can you add the high res version to public/assets just so we have it?
logo-lucha-libre-high-res

and sidewalk-logo-new can be removed from public/assets

@MattJ12
Copy link
Collaborator Author

MattJ12 commented Jun 4, 2020

Okay, so on mobile the mask doesn't look any good unless we improve the resolution. I played around with it and got a resolution I'm happy with. It's like 45 kb instead of 15 kb, but I think that's fine. Worth it to have a crisp logo.

I'm attaching it here. Could you add it for the two Mexican cities, and then also create an edited version for the other cities that is at that resolution?

sidewalk-logo

Ok, I added the 45kb of both the logos to their respective city folders, took out the sidewalk-logo-new and added the high res version of the logo to the public/assets folder.

@misaugstad
Copy link
Member

Great! Looks good. Last thing: we already have the high res version of the logo without the mask in the assets folder, called "logo-high-res.png". Could you just rename the lucha libre one to have the same convention (and also specify that it is not our normal logo) by renaming it "logo-lucha-libre-high-res.png"? I think that's the last thing I have :)

@MattJ12
Copy link
Collaborator Author

MattJ12 commented Jun 4, 2020

Great! Looks good. Last thing: we already have the high res version of the logo without the mask in the assets folder, called "logo-high-res.png". Could you just rename the lucha libre one to have the same convention (and also specify that it is not our normal logo) by renaming it "logo-lucha-libre-high-res.png"? I think that's the last thing I have :)

Sounds good, I just changed it.

Copy link
Member

@misaugstad misaugstad left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

@misaugstad misaugstad merged commit c3243b3 into develop Jun 6, 2020
@misaugstad misaugstad deleted the 2086-logo-add-lucha-libre-mask branch June 6, 2020 01:56
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.

2 participants