Skip to content
This repository has been archived by the owner on Oct 18, 2022. It is now read-only.

[CRAI-365] Extracting code from app #2

Merged
merged 10 commits into from Jan 3, 2019
Merged

[CRAI-365] Extracting code from app #2

merged 10 commits into from Jan 3, 2019

Conversation

anyashka
Copy link
Contributor

@anyashka anyashka commented Jan 3, 2019

Ticket

CRAI-365

Task Description

This task contains extracting code from the CarLens application. It was mostly copied from that place and slightly changed so it could be used outside the app too.
It should be used as follows:

  • The developer should initialize CarlensCollectionViewLayout() and assign it to their collection view layout.
  • The developer should subclass CarlensCollectionViewCell and initialize there topView and cardView on a start. It should be done with the help of configure method. If he doesn't a view will be simply empty.

Aditional Notes (optional)

It is a minimum viable product at the moment. Eventually, more generic features can be added.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some little suggestions, great job!

}

private func animateViews(toProgress progress: Double) {
guard cardView != nil else { return }
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why cardView is implicitly unwrapped optional, if it can be nil here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, I know it looks weird. The reason behind this is that I didn't want the developer to use a custom initializer, so they could use dequeueReusableCell. So, they need to call configure function at the start. However, if they don't do that, views can actually be nil.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but what is the advantage of implicitly unwrapped optional over optional there?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's perfect that you asked and pointed that out. Actually, here there shouldn't be guard as well. We have two options:

  1. Leaving implicitly unwrapped optional and removing guard. So that it will crash if a developer doesn't initialize views.
  2. Setting it to optional and then adding guards everywhere and simply showing an empty view.

I think both options are fine. What do you think?
Maybe even the first one is better because in that way they know something gone really wrong :D

Copy link

@ghost ghost Jan 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am also rooting for the first option :D

guard let attributes = super.copy(with: zone) as? CarlensLayoutAttributes else { return super.copy(with: zone) }
attributes.progress = progress
return attributes
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about use of optional chaining here? Because you still want to return the same value even if casting fails. For example:
override func copy(with zone: NSZone?) -> Any {
let attributes = super.copy(with: zone)
(attributes as? CarlensLayoutAttributes)?.progress = progress
return attributes
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perfect suggestion, thanks!

/// SeeAlso: UICollectionViewLayoutAttributes
override func isEqual(_ object: Any?) -> Bool {
guard let attributes = object as? CarlensLayoutAttributes else { return false }
guard attributes.progress == progress else { return false }
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe make these two guards into one?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For sure, thanks for spotting this out 👍

@anyashka anyashka merged commit 74364eb into develop Jan 3, 2019
@anyashka anyashka deleted the task/CRAI-365 branch January 3, 2019 14:45
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant