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

Changed the icon color on swipe to contrast the background #78

Merged
merged 1 commit into from
Oct 26, 2019

Conversation

ragone
Copy link

@ragone ragone commented Oct 26, 2019

As a user, when I swipe a header, I want the icon to contrast the background, so that better see the action taken

chrome-capture

@ragone ragone force-pushed the style/swipe-color branch from c0d7910 to c397a6f Compare October 26, 2019 14:56
@munen
Copy link
Collaborator

munen commented Oct 26, 2019

Hi @ragone

Thank you very much for your great PR! I really appreciate the way you did it - with a user story and GIF. That makes it very easy to review.

I also like the feature very much - it makes a lot of sense! So I definitively want to merge it. It's just not working on my machine, though^^

I made a GIF on how I tested it. Do you see something amiss with my test or did you maybe not push everything?

In any case, let's get this to work and let's merge it in!

demo

@ragone
Copy link
Author

ragone commented Oct 26, 2019

Thanks @munen! Happy to contribute :)

I think there might be a misunderstanding of what the change is, as it looks like it is working for you. The change is simply making the icon go from black to white at the same transition as the background-color.

The background itself is unchanged. I hope that makes sense.

@munen
Copy link
Collaborator

munen commented Oct 26, 2019

Hm.. you are absolutely right! I was misreading the GIF, but your user story is actually very clear on what's happening. And you're right, it works as expected and it is a great addition!

To elaborate on my mistake: I expected the background for the toggled headline to be grayed out as it shows on your GIF. However, you probably just clicked it before you started the toggle(;

In any case, your change totally works for me (as my GIF shows). The code also makes sense now^^ And improving the icon contrast definitively is a good change. Sorry for the initial mistake. It's a definitive merge!

Thanks, again! 🙏

@munen munen merged commit 0cdd6d0 into 200ok-ch:master Oct 26, 2019
@munen
Copy link
Collaborator

munen commented Oct 26, 2019

@ragone FYI, I happily added your contributions with attribution to the changelog: a4df7e4

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