Explore converting SWTableViewCell to Carthage

RESOLVED FIXED

Status

()

RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: fluffyemily, Assigned: farhan)

Tracking

unspecified
All
iOS

Firefox Tracking Flags

(fxios+)

Details

Attachments

(2 attachments)

(Reporter)

Description

3 years ago
Can we make SWTableViewCell Carthage compliant, or can we find a Carthage compliant alternative?
(Reporter)

Updated

3 years ago
Depends on: 1237573
(Reporter)

Updated

3 years ago
Blocks: 1223403
Rank: 6
tracking-fxios: ? → +
Hardware: Other → All
(Reporter)

Updated

3 years ago
No longer blocks: 1223403
(Assignee)

Comment 1

2 years ago
I've spent some time looking for an alternative and found one in the default iOS SDK. This is the same approach that a lot of the built in iOS apps (like mail/Notes) use. 

The UI is slightly different and so I'd like to get Robin to have a look first to make sure its okay. 

I've attached a small video that has both the current behavior and then the new behavior. If this new behavior is acceptable we can remove about 1300 lines of code. ヽ(•‿•)ノ
Flags: needinfo?(randersen)
(Assignee)

Comment 2

2 years ago
Created attachment 8765549 [details]
readinglist.mov

A before and after of the ReadingList action buttons.
This looks great, Farhan! GFI!
Flags: needinfo?(randersen)
(Assignee)

Comment 4

2 years ago
Created attachment 8767749 [details] [review]
Pull Request
Attachment #8767749 - Flags: review?(etoop)
(Reporter)

Comment 5

2 years ago
Comment on attachment 8767749 [details] [review]
Pull Request

Super nice. And I cannot cheer enough about the removal of a dependency, especially from ThirdParty.

I would normally ask you to update the UITests (ReaderViewUITests) to reflect these changes in functionality (swipe left to reveal Mark As Read instead of swipe right) but seeing as those tests are broken because the accessibility label is an attributed string and Kif can't perform actions on those, then there doesn't seem much point. 

If you feel the need, you _could_ reimplement ReaderViewUITests in UITesting in a new target and actually get them to work, but don't feel obliged.
Attachment #8767749 - Flags: review?(etoop) → review+
(Assignee)

Comment 6

2 years ago
master f8c4e23633989fb79c62f174b21e8d3182779a7a
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
(Assignee)

Updated

2 years ago
Assignee: nobody nobody → fpatel fpatel
You need to log in before you can comment on or make changes to this bug.