Allow to set a background image to table view and cells views #416

phimage · 2017-03-25T11:15:55Z

i.e. view with backgroundView attribute
Add also BlurDesignable to table view to blur the backgroundView

A lot of time I need to put a background image on this ui components without adding a specific UIImageView. With the same principle of BlurDesignable a view is added. But this time not in subviews hierarchy, with crapping things like scroll view, etc... but the special attribute backgroundView
to see background of a table, each cell view must be configured with clear color (or some transparency )
UICollectionView could also conform to BackgroundImageDesignable but there is no AnimatableCollectionView yet, any reason?
I must edit the change log on my own? could be weird with conflicts when there is multiple PR

i.e. view with `backgroundView` attribute Add also BlurDesignable to table view to blur the background view

IBAnimatableBot · 2017-03-25T11:21:29Z

	1 Error
🚫	Any changes to library code need a summary in the Changelog.

	1 Warning
⚠️	Consider adding supporting documentation to this change. Documentation can be found in the `docs` directory.

Generated by 🚫 Danger

tbaranes

I made a first quick review of this, just a few small comments, if you can take a look, I will make a second review after that.

There's also the CHANGELOG and documentation entry missing.

tbaranes · 2017-03-27T07:14:48Z

IBAnimatable/BackgroundImageDesignable.swift

+public extension BackgroundImageDesignable where Self: BackgroundDesignable {
+  public func configureBackgroundImage() {
+    if let image = backgroundImage {
+      if let imageView = self.backgroundView as? AnimatableImageView {


That self. isn't mandatory, is it? As the others below

self is not mandatory I think (there is no closure)
I put self when possible to avoid some issue when renaming stuff and local variable become class variable etc...
But there is poor or con, and swift lint do not have this rule yet
realm/SwiftLint#321

Like I do in previous PR I will remove it

@phimage It is a controversial topic for keeping self or not. We try to omit self if possible for our repo, can you also follow that?

tbaranes · 2017-03-27T07:16:04Z

IBAnimatable/BackgroundImageDesignable.swift

+extension UICollectionViewCell: BackgroundDesignable {}
+extension UICollectionView: BackgroundDesignable {}
+
+fileprivate typealias BackgroundImageView = AnimatableImageView


Not sure about that typealias, I feel that bring more confusion than using directly AnimatableImageView.

I am thinking about doing something like

class PrivateImageView: AnimatableImageView {}

so making typealias allow to change quickly

but I can remote it

I don't see advantages to not use AnimatableImageView directly here, is there a reason?

agree with @tbaranes for using AnimatableImageView instead of typealias BackgroundImageView.

On the other hand for the private subclass. I think creating a private subclass is good for some case I can think of. For example, we can check like backgroundView as? PrivateImageView then we know backgroundView must be set within extension BackgroundImageDesignable. Another someone may use AnimatableImageView for backgroundView outside of IBAnimatable framework. And we should not reset backgroundView = nil if backgroundView is not original set by extension BackgroundImageDesignable.

Is it the case you are thinking of, @phimage ? I leave it to you to make a decision to use private class PrivateAnimatableImageView: AnimatableImageView or not.

tbaranes · 2017-03-27T07:16:39Z

IBAnimatable/BlurDesignable.swift

@@ -28,11 +28,14 @@ public extension BlurDesignable where Self: UIView {
   configureBlurEffectStyle method, should be called in layoutSubviews() method
   */
  public func configureBlurEffectStyle() {
+    configureBlurEffectStyle(self)
+  }
+  public func configureBlurEffectStyle(_ blurableView: UIView) {


newLine missing

shall we use public func configureBlurEffectStyle(for blurableView: UIView)? using for instead of _ there.

tbaranes · 2017-03-27T07:18:52Z

@phimage Thanks for doing this! It's true that a common behavior in iOS development.

UICollectionView could also conform to BackgroundImageDesignable but there is no AnimatableCollectionView yet, any reason?

Just needed someone to ask for it 😬
There's no reason to not have it, just no one take the time to do it or need it until now.

I must edit the change log on my own? could be weird with conflicts when there is multiple PR>

Yeah, that's annoying part with the CHANGELOGs

JakeLin

@phimage well done on this PR, can you please address the comment from @tbaranes , and we should be ready to go after that.

JakeLin · 2017-03-27T22:11:51Z

IBAnimatable/BackgroundImageDesignable.swift

+extension UICollectionViewCell: BackgroundDesignable {}
+extension UICollectionView: BackgroundDesignable {}
+
+fileprivate typealias BackgroundImageView = AnimatableImageView


agree with @tbaranes for using AnimatableImageView instead of typealias BackgroundImageView.

On the other hand for the private subclass. I think creating a private subclass is good for some case I can think of. For example, we can check like backgroundView as? PrivateImageView then we know backgroundView must be set within extension BackgroundImageDesignable. Another someone may use AnimatableImageView for backgroundView outside of IBAnimatable framework. And we should not reset backgroundView = nil if backgroundView is not original set by extension BackgroundImageDesignable.

Is it the case you are thinking of, @phimage ? I leave it to you to make a decision to use private class PrivateAnimatableImageView: AnimatableImageView or not.

JakeLin · 2017-03-27T22:13:42Z

IBAnimatable/BackgroundImageDesignable.swift

+public extension BackgroundImageDesignable where Self: BackgroundDesignable {
+  public func configureBackgroundImage() {
+    if let image = backgroundImage {
+      if let imageView = self.backgroundView as? AnimatableImageView {


@phimage It is a controversial topic for keeping self or not. We try to omit self if possible for our repo, can you also follow that?

JakeLin · 2017-03-27T22:17:29Z

IBAnimatable/BlurDesignable.swift

@@ -28,11 +28,14 @@ public extension BlurDesignable where Self: UIView {
   configureBlurEffectStyle method, should be called in layoutSubviews() method
   */
  public func configureBlurEffectStyle() {
+    configureBlurEffectStyle(self)
+  }
+  public func configureBlurEffectStyle(_ blurableView: UIView) {


shall we use public func configureBlurEffectStyle(for blurableView: UIView)? using for instead of _ there.

JakeLin · 2017-03-27T22:21:03Z

IBAnimatable/BackgroundImageDesignable.swift

+extension UITableViewHeaderFooterView: BackgroundDesignable {}
+extension UITableView: BackgroundDesignable {}
+extension UICollectionViewCell: BackgroundDesignable {}
+extension UICollectionView: BackgroundDesignable {}


It took me a while to understand this, I found out is a great idea for having a protocol BackgroundDesignable Because we can't use protocol extension for conforming different classes like public extension BackgroundImageDesignable where Self: UITableViewCell or UITableViewHeaderFooterView or UITableView or UICollectionViewCell or UICollectionView. To have BackgroundDesignable and make the code looks better. Well done 👍

JakeLin · 2017-03-27T22:29:21Z

@phimage thank you very much for a great PR. @tbaranes has answered your questions. I have 2 cents for them.

We do need AnimatableCollectionView and AnimatableCollectionViewCell, we can see there is a card/issue for that #53. But as @tbaranes said, we didn't do it because no one asked for. It will be great if you can offer some hand on it. BackgroundImageDesignable is great for AnimatableCollectionView and AnimatableCollectionViewCell as well.

For the CHANGELOG.md, it is annoying, we have to rebase every time before we merge into master. But good for the user to check all changes. Please modify it when you submit a PR.

@phimage please let me know if you would like to offer some small helps on this project and join IBAnimatable GitHub organisation as an internal contributor. We have a Slack channel for all internal contributors as well. Thanks again for all you contributions, you make IBAnimatable better 💪

phimage · 2017-03-28T08:14:40Z

I will make the change after finishing with #418

@JakeLin ok for organisation and Slack
I have some other topics that I want to add to IBAnimatable, and some like buttons touch animation must be discussed

JakeLin · 2017-03-28T11:18:05Z

@phimage welcome onboard. I have sent you an invite to be an internal contributor of the IBAnimatable organisation. Our rule is simple: There is no pressure to work on the project. Everyone has the same access to the repo. We use feature branch like feature/animatable-collection-view or bugfix/fix-corner-radius and PR to master. We are open to any opinions. Once we have one or two 👍 , we can merge the PRs. Please shout out if you have any question.

Can you also please send me your Slack account (email)? You can find my email on my GitHub profile page. Thanks

…mage # Conflicts: # IBAnimatable.xcodeproj/project.pbxproj # IBAnimatable/AnimatableCollectionViewCell.swift # IBAnimatable/AnimatableTableViewCell.swift

JakeLin

👍 well done.

JakeLin · 2017-04-02T04:45:57Z

@phimage I merge it now, Travis is failing because of bundle exec danger. But I can see the app has been built successfully. Thanks for your PR, well done 👍

tbaranes · 2017-04-02T07:44:45Z

There's a documentation and example app update missing, should we add an issue? Pretty sure we will forget about this otherwise 😬

I added an entry in #238 for the example app

Allow to set a background image to table view and cells views

c501a5e

i.e. view with `backgroundView` attribute Add also BlurDesignable to table view to blur the background view

tbaranes requested changes Mar 27, 2017

View reviewed changes

JakeLin reviewed Mar 27, 2017

View reviewed changes

JakeLin added the type: enhancement label Mar 28, 2017

JakeLin added this to the 4.0 milestone Mar 28, 2017

Merge remote-tracking branch 'origin/master' into feature-backgroundi…

331a895

…mage # Conflicts: # IBAnimatable.xcodeproj/project.pbxproj # IBAnimatable/AnimatableCollectionViewCell.swift # IBAnimatable/AnimatableTableViewCell.swift

JakeLin approved these changes Apr 2, 2017

View reviewed changes

JakeLin merged commit e7cf528 into IBAnimatable:master Apr 2, 2017

lastMove mentioned this pull request Apr 2, 2017

IBAnimatable App #238

Open

11 tasks

phimage mentioned this pull request Apr 2, 2017

BackgroundImageDesignable not working #428

Closed

tbaranes modified the milestones: 4.x, 4.0 Apr 20, 2017

phimage deleted the feature-backgroundimage branch April 22, 2018 10:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow to set a background image to table view and cells views #416

Allow to set a background image to table view and cells views #416

phimage commented Mar 25, 2017 •

edited

IBAnimatableBot commented Mar 25, 2017 •

edited

tbaranes left a comment •

edited

tbaranes Mar 27, 2017

phimage Mar 27, 2017 •

edited

JakeLin Mar 27, 2017

tbaranes Mar 27, 2017

phimage Mar 27, 2017

tbaranes Mar 27, 2017

JakeLin Mar 27, 2017

tbaranes Mar 27, 2017

JakeLin Mar 27, 2017

tbaranes commented Mar 27, 2017

JakeLin left a comment

JakeLin Mar 27, 2017

JakeLin Mar 27, 2017

JakeLin Mar 27, 2017

JakeLin Mar 27, 2017

JakeLin commented Mar 27, 2017

phimage commented Mar 28, 2017

JakeLin commented Mar 28, 2017

JakeLin left a comment

JakeLin commented Apr 2, 2017

tbaranes commented Apr 2, 2017

Allow to set a background image to table view and cells views #416

Allow to set a background image to table view and cells views #416

Conversation

phimage commented Mar 25, 2017 • edited

IBAnimatableBot commented Mar 25, 2017 • edited

tbaranes left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phimage Mar 27, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tbaranes commented Mar 27, 2017

JakeLin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JakeLin commented Mar 27, 2017

phimage commented Mar 28, 2017

JakeLin commented Mar 28, 2017

JakeLin left a comment

Choose a reason for hiding this comment

JakeLin commented Apr 2, 2017

tbaranes commented Apr 2, 2017

phimage commented Mar 25, 2017 •

edited

IBAnimatableBot commented Mar 25, 2017 •

edited

tbaranes left a comment •

edited

phimage Mar 27, 2017 •

edited