rubyforgood / habitat_humanity

MIT License
6 stars 13 forks source link

add icons for various browsers #89

Closed bjmllr closed 8 years ago

bjmllr commented 8 years ago

Resolves #88

The preferred blue-and-green-on-white design is used for the public-facing form, while a black-on-white variant is used in the admin dashboard to provide a visual cue as suggested in #88 .

I used rails_real_favicon to generate these icons from the EPS files provided by the org, which I converted to SVG to serve as a "master" icon.

leesharma commented 8 years ago

Looks good! Two suggestions/questions though:

leesharma commented 8 years ago

I saved the "check-in" and "admin dashboard" pages to my iPhone (from the right branch this time–sorry!). They look sharp! Nice work @bjmllr.

icon on iPhone

The admin page is still showing up as the color icon though, so we should figure that out.

bjmllr commented 8 years ago

Rebased and fixed the home screen issue ... at least on android. I think I may have fixed it for iOS as well but I can't remember I left an iProduct to test with. Haven't yet removed the extra gem nor inverted the b&w icon.

leesharma commented 8 years ago

Nice! It looks good on my iPhone too.

img_2908

leesharma commented 8 years ago

Oh! I thought of something else. The administrate views use the black icon, but the sign in and report views still use the full-color one (because they don't use the admin layout). Do you all think that's an issue/worth addressing now? It is inconsistent, but those icons aren't likely to be saved on a phone screen.

bjmllr commented 8 years ago

Inverted the admin icon and removed the gemfile entry.

I think it would be nice to have all the admin views using views/layouts/admin/application.html.erb instead of views/layouts/application.html.erb. That would solve the icon issue.

leesharma commented 8 years ago

@bjmllr That makes sense–it'll provide a consistent UI too, which is nice.

leesharma commented 8 years ago

I added some code that just puts the right icons on the non-administrate admin screens. I'm not entirely comfortable with this solution because of the methods on ApplicationController, but doing that allows us to keep all the layout-related code in one place.

Because of Devise, I can't see an easy way to add this in an AdminController parent class or with an ActsAsAdmin module (without overriding all of those controllers).

I initially tried to get these to work with the administrate/application layout we already have, but there were a lot of gotchas; I worry that even if we get it working now, the hacks required to get it working will be confusing down the line.

Thoughts/ideas of a better way to do this?

jdsayle commented 8 years ago

I'm wondering if the fact that administrate is an engine is causing the icon issues? Just a guess on my end . . . Lee, will you be at Arlington Ruby on Wednesday? If so, I'd like to try and tackle this issue together in person there.

-JD

On Fri, Jul 29, 2016 at 11:16 PM, Lee Sharma notifications@github.com wrote:

I added some code that just puts the right icons on the non-administrate admin screens. I'm not entirely comfortable with this solution because of the methods on ApplicationController, but doing that allows us to keep all the layout-related code in one place.

I initially tried to get these to work with the administrate/application layout we already have, but there were a lot of gotchas; I worry that even if we get it working now, the hacks required to get it working will be confusing down the line.

Thoughts/ideas of a better way to do this?

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/rubyforgood/habitat_humanity/pull/89#issuecomment-236337386, or mute the thread https://github.com/notifications/unsubscribe-auth/AM9dB27KWQp01cNm8i97KdKgkKAy0DEkks5qasICgaJpZM4JS-aV .

leesharma commented 8 years ago

I could be!

Edit: Sure, I will be. I realize my response is a bit ambiguous.

leesharma commented 8 years ago

The other thing to consider is that using Administrate is supposed to save time; at the moment, it seems to be costing time/not letting us do what we want.

It might be worth coming up with some basic fix for this and then rolling our own admin panel.

jdsayle commented 8 years ago

Agreed! I'll do some research on my end, but it's looking like administrate may not be the best option long term and I think you made a great point earlier about things getting confusing down the line when the app gets into the maintenance part of it's life cycle.

On Sat, Jul 30, 2016 at 1:27 PM, Lee Sharma notifications@github.com wrote:

The other thing to consider is that using Administrate is supposed to save time; at the moment, it seems to be costing time/not letting us do what we want.

It might be worth coming up with some basic fix for this and then rolling our own admin panel.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/rubyforgood/habitat_humanity/pull/89#issuecomment-236377897, or mute the thread https://github.com/notifications/unsubscribe-auth/AM9dB6ddUUP9OmEdwKvbsJOxKVg8Wb1pks5qa4lsgaJpZM4JS-aV .

bjmllr commented 8 years ago

What was the issue with devise? We can apply a mix-in to devise's controllers in an initializer.

leesharma commented 8 years ago

Oh, neat–I didn't know you could do that! A devise initializer + adding layout (or a module) to the in-app controllers is a much neater solution.

Even with that though, Administrate is limiting what we're able to do (in terms adding "composite" views, etc.), and a lot of the customizations feel hacky. I think it may still be worth moving away from it, even if it's not a priority at the moment.

leesharma commented 8 years ago

I changed the code to use the initializer instead of ApplicationController, but it adds complexity in a different area: the admin controller name is listed in several places, and adding admin controllers means we need to list it again.

It's probably not worth wrapping in an ApplicationController method, module, or new parent class yet (since we only have two non-engine admin controllers), but we may need to in the future.

bjmllr commented 8 years ago

Acknowledging that we might rethink our approach to the admin areas later, is this ready for merge?

leesharma commented 8 years ago

@bjmllr Are you happy with the icons, or should we do something different for the admin browser logo?

bjmllr commented 8 years ago

I think all the icons are good enough. We can always change them later based on feedback.

leesharma commented 8 years ago

All right then–looks good to me!