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

issue#10: Added preview to posts on FeedViewController #53

Merged
merged 9 commits into from
Feb 24, 2019
Merged

issue#10: Added preview to posts on FeedViewController #53

merged 9 commits into from
Feb 24, 2019

Conversation

RJ-Clegg
Copy link
Collaborator

@RJ-Clegg RJ-Clegg commented Feb 23, 2019

Issue #10

Changes made:

  • Added a new extension: String+Extension.swift with a function called strippedHtml

This was needed as I noticed the API brings back a lot of HTML in some texts that we display.

  • Reconfigured ListingTableViewCell to have an UIImage / Body text and title text. Using UIStackView 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 in FeedViewController - no longer do we need to have logic in the UITableViewDataSource method cellForRowAt to decide which UITableViewCell 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, using UIStackView.

  • Added a static var to the ListingTableViewCell 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 than cell.display(listing: listing)

  • Made the @IBOutlet's private in ListingTableViewCell - no need to expose them, if we don't have to

Edit

Urgh, sorry for the extra commits; bunch of extra stuff I noticed after the original commit. No more commits now.

import Foundation

extension String {
var stripHtml: String {
Copy link
Owner

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

Copy link
Collaborator Author

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.

Copy link
Owner

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
Copy link
Owner

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.

Copy link
Collaborator Author

@RJ-Clegg RJ-Clegg Feb 24, 2019

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

Copy link
Owner

@kgellci kgellci left a 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.

@RJ-Clegg RJ-Clegg added this to In progress in Area51 via automation Feb 24, 2019
@RJ-Clegg RJ-Clegg merged commit 4ca0044 into kgellci:master Feb 24, 2019
Area51 automation moved this from In progress to Done Feb 24, 2019
@RJ-Clegg RJ-Clegg deleted the issue10 branch February 24, 2019 06:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Area51
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants