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

Add customizable background color param #47

Closed
wants to merge 2 commits into from
Closed

Add customizable background color param #47

wants to merge 2 commits into from

Conversation

hyouuu
Copy link
Contributor

@hyouuu hyouuu commented Oct 28, 2016

As well as changing blurStyle param from optional to required as we always provide a default value, and format the init calls.

The initiative is that a friend said he felt the gray blurred background feels like coming from nowhere and somewhat distracting, so I'm customizing my app to use black background just like how most photo viewers behave

As well as changing blurStyle param from optional to required as we always provide a default value, and format the init calls
@JanGorman
Copy link
Owner

I was away on vacation – sorry for the delay. I'll take a look at this later today

@JanGorman
Copy link
Owner

@hyouuu So I finally got round to looking at this. I like the idea but really the background color and background blur are mutually exclusive options, i.e. when you set a color you don't see or should set a blur. The initialisers should reflect that as otherwise they suggest that you can set a coloured blur.

I'd modify this to create separate convenience initialisers that make this explicit and then also make sure that if a colour is set, no blur is applied to save the system from doing work that's not needed. Then we're good to go.

@hyouuu
Copy link
Contributor Author

hyouuu commented Dec 5, 2016

Makes sense - I'll change it when have a chance; feel free to change on top of my changes if you get free time ;)

@JanGorman JanGorman closed this in c452502 Dec 28, 2016
@hyouuu
Copy link
Contributor Author

hyouuu commented Jan 3, 2017

Awesome thanks!!

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