search

CatimaLoyalty / Android

Catima, a Loyalty Card & Ticket Manager for Android
https://catima.app
GNU General Public License v3.0
841 stars 151 forks source link

Star icon should be light in main view on dark theme #72

Closed TheLastProject closed 3 years ago

TheLastProject commented 3 years ago

Very minor little thing:

image

arshbeerSingh commented 3 years ago

Can I work on this issue?

TheLastProject commented 3 years ago

Sure! :) Is this related to Hacktoberfest? Just asking so I can add a hacktoberfest-accepted label so your pull request counts for that.

TheLastProject commented 3 years ago

It works correctly in the LoyaltyCardViewActivity by the way, so you can probably copy some logic from there. Although if you end up copying it I'd of course rather you move it to a place both the LoyaltyCardViewActivity and the MainActivity can use it :)

arshbeerSingh commented 3 years ago

@TheLastProject It is not related to Hacktoberfest. I am taking part in that event and I have already fulfilled its requirements. I was looking to fix some easy beginner issues as I am new to contributing to open-source projects :-) Also, can you please assign this issue to me?

TheLastProject commented 3 years ago

Done :)

arshbeerSingh commented 3 years ago

Hi @TheLastProject , I tried to fix this issue in two ways:

  1. changing the star icon to a light colour in dark mode
  2. changing the star icon to golden shiny colour (same colour for both light and dark themes)

I think that the second option is better as it represents the favourite icon in a better way and at the same time reduces code complexity. But, I would also like your opinion on what option do you think should be finalized as you are the project owner. (Please refer to the attached screenshots) white golden_1 golden_2

TheLastProject commented 3 years ago

I personally prefer option 1, as it is more consistent with other UI elements and icons and fits the Android design guidelines. I definitely do see value in your suggestion though and I do really appreciate your thinking along like this. Still, option 1 please :)

arshbeerSingh commented 3 years ago

Sure, I do agree with your preference and would submit a pull request soon with option 1 implemented. :+1:

TheLastProject commented 3 years ago

Awesome, thank you :)

TheLastProject commented 3 years ago

Fixed in #75.