-
Notifications
You must be signed in to change notification settings - Fork 31
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
issue#10: Added preview to posts on FeedViewController #53
Conversation
…sing error with selfText in Listing.swift - apparently it can be nil, not just empty
import Foundation | ||
|
||
extension String { | ||
var stripHtml: String { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit, since it returns a new string, should be called strippingHTML
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally, functions aren't named by what they're doing, more towards their post-actions or action-to-be-done.
An example of this is in the Swift Standard library - for Float
and Double
- which have two functions; round() and rounded()
Using the above example, I feel stripHtml
is just as appropriate as strippedHtml
however I don't feel strippingHtml
is the right choice here.
I agree though, it should have a different name, since it returns a new string - if we follow the Standard library example, strippedHtml
would seem more appropriate to me.
I'll make this change before merging.
I am however open to discussing this more though, off this PR. Maybe I am missing something more obvious.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Either one sounds good
|
||
private extension ListingTableViewCell { | ||
func configureTitleText(with listing: Listing) { | ||
titleLabel.text = listing.title.stripHtml |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In a later PR, we can create ViewModels which already hold the preprocessed data.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
100% agree with this. I generally use the MVVM pattern with my projects. I'll create an issue for this and solve it in a upcoming PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a few small comments but looks good to merge.
…conventions more closely
Issue #10
Changes made:
String+Extension.swift
with a function calledstrippedHtml
This was needed as I noticed the API brings back a lot of HTML in some texts that we display.
ListingTableViewCell
to have an UIImage / Body text and title text. UsingUIStackView
makes showing and hiding the different UI elements trivial (Just hide views you don't want to show and that's it!) and cleans up the code inFeedViewController
- no longer do we need to have logic in theUITableViewDataSource
methodcellForRowAt
to decide whichUITableViewCell
we should be using.Another advantage was not having to use optional casting, to call a method that the
UITableViewCell
implements - eg via the following protocol:ListingDisplayable
So at the call site this:
(cell as? ListingDisplayable)?.display(listing: listing)
becomes this:
cell.display(listing)
Furthermore, if we should need to add more UI elements to our
ListingTableViewCell
it will be trivial and scale easy enough, usingUIStackView
.Added a
static var
to theListingTableViewCell
for the reuseIdentifier - this allows us to avoid using 'string-api' which is very error prone. We get compile time safety this way.Removed the
ListingThumbnailTableViewCell
as it became redundant.Made a small change to
ListingDisplayable
protocol - I felt that at the call site: cell.display(listing)
was better thancell.display(listing: listing)
Made the
@IBOutlet
's private inListingTableViewCell
- no need to expose them, if we don't have toEdit
Urgh, sorry for the extra commits; bunch of extra stuff I noticed after the original commit. No more commits now.