Skip to content

add branded organization logo #466

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Mar 14, 2019
Merged

add branded organization logo #466

merged 2 commits into from
Mar 14, 2019

Conversation

Morantron
Copy link
Collaborator

@Morantron Morantron commented Feb 28, 2019 •
edited by enricostano
Loading

How to test

  • Open a rails console bundle exec rails c
  • To show logo for organization with id 1, run Redis.current.set('branded_organization_id', 1)
  • Reboot the app, in staging sudo systemctl restart timeoverflow

@Morantron Morantron force-pushed the feature/add-redeira-logo branch from 0b1fc6f to 049eeed Compare February 28, 2019 10:58
@enricostano
Copy link
Contributor

@sseerrggii In which project this PR should be? 1T, 2T?

@sseerrggii
Copy link
Contributor

@sseerrggii In which project this PR should be? 1T, 2T?

1T

@sseerrggii
Copy link
Contributor

Tested, works fine for me 👍

Waiting for a redeira approval

@sseerrggii
Copy link
Contributor

a redeira approve ✔️

markets
markets reviewed Mar 1, 2019
View reviewed changes
app/views/application/_brand_logo.html.erb Outdated
@@ -0,0 +1,3 @@
<div class="container text-center" style="margin-bottom: 20px">
<%= image_tag("redeira@1x.png", class: 'organization-brand-logo', srcset: '/assets/redeira@2x.png 2x') %>
Copy link
Collaborator

@markets markets Mar 1, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

@Morantron are you sure the srcset works with compiled assets?

@markets
Copy link
Collaborator

markets commented Mar 1, 2019 •
edited
Loading

I think it would be a good idea to squash this PR 🙏 😛

@sseerrggii
Copy link
Contributor

Wait before merge. A redeira team is thinking about the right logo

enricostano
enricostano reviewed Mar 4, 2019
View reviewed changes
app/helpers/brand_logo_helper.rb Outdated
@@ -7,6 +7,7 @@ def render_brand_logo
private

def should_render_logo?
byebug
Copy link
Contributor

Choose a reason for hiding this comment

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

Elegant.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

🎩 👌

@sseerrggii
Copy link
Contributor

Logo A Redeira Azul

This one @Morantron

@Morantron Morantron force-pushed the feature/add-redeira-logo branch from cc74fd2 to b06243e Compare March 13, 2019 08:06
@Morantron
Copy link
Collaborator Author

new logo is on 💃

image

it's as big as the provided image

@sseerrggii
Copy link
Contributor

Perfect @Morantron

Can we add "img-responsive" to a img class to avoid image overflow in small screens?

And... Why don't pass the test?? The Travis CI build failed 🤔

@Morantron Morantron force-pushed the feature/add-redeira-logo branch from b06243e to cac03ec Compare March 13, 2019 18:46
@Morantron
Copy link
Collaborator Author

pushing again with the new class, and rebased with latest develop ( this might fix travis thing )

@enricostano
Copy link
Contributor

@Morantron @sseerrggii how it looks like with mobile app?

@Morantron Morantron force-pushed the feature/add-redeira-logo branch from cac03ec to 56612cc Compare March 14, 2019 10:57
@Morantron
Copy link
Collaborator Author

haven't tried in the mobile app, but in small screens looks good ™️

image

@sseerrggii
Copy link
Contributor

sseerrggii commented Mar 14, 2019 •
edited
Loading

Tested, work fine, showed in 1 organization and don't in others

I tested also in app, I have this first screen and I was shocked 😱 , after that I used the app and everyhing works fine, when I reopened the app this first screen works fine... strange thing 🤔 Maybe is possible to render homepage with user logged in? I think it's something that happens only when you reinstall the app in a phone that had previously installed app and logged in, so I think fix is not needed for the moment.

This is the fist screen:

Screenshot_20190314-141553

Logo at footer:

Screenshot_20190314-142858

And home at log out:

Screenshot_20190314-143126

@enricostano @Morantron
Update app/helpers/brand_logo_helper.rb
f8e1f5e 
Co-Authored-By: Morantron <jorge@morante.eu>
@sseerrggii
Copy link
Contributor

I created new issue for problems with app #474

@enricostano enricostano merged commit d0f2bbf into develop Mar 14, 2019
@enricostano enricostano deleted the feature/add-redeira-logo branch March 14, 2019 18:11
@enricostano enricostano added this to the 1.12.0 milestone Mar 14, 2019
@enricostano enricostano mentioned this pull request Mar 14, 2019
Merged
@sauloperez
Copy link
Collaborator

Not bad. Quite professional 👌

@markets markets mentioned this pull request Apr 30, 2021
Closed
@markets markets mentioned this pull request Feb 13, 2023
Merged
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@markets markets markets left review comments

@enricostano enricostano enricostano approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.12.0
Development

Successfully merging this pull request may close these issues.
5 participants
@Morantron @enricostano @sseerrggii @markets @sauloperez