Skip to content
This repository has been archived by the owner on Sep 20, 2023. It is now read-only.

Allow for quick toggle between open/closed issues #2687

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Huddie
Copy link
Collaborator

@Huddie Huddie commented Mar 13, 2019

Currently, a WIP, looking for some feedback.
Closes #374

Quick description:

During Editing:
Segment Bar recedes allowing easy typing

When Finished Editing:
Segment Bar appears allowing a quick move between closed/open issues

It currently does update the state properly and the list does refresh properly

@Huddie
Copy link
Collaborator Author

Huddie commented Mar 13, 2019

Simulator Screen Shot - iPhone XR - 2019-03-12 at 21 11 11
Simulator Screen Shot - iPad Pro (11-inch) - 2019-03-12 at 21 10 07
Simulator Screen Shot - iPad Pro (11-inch) - 2019-03-12 at 21 10 02

@Huddie Huddie added 🚧 wip Work in progress 💤 awaiting review Pull Request is awaiting code reviews labels Mar 13, 2019
@wayni208
Copy link
Contributor

That’s inspired work!

Sent with GitHawk

@ijm8710
Copy link

ijm8710 commented Mar 13, 2019

Awesome @Huddie

3 f/u

  • Does hitting open again, deselect it allowing for both open and closed to show
  • if one manually types is:open or is:closed does it automatically convert to the use of the filter?
  • Would you agree that this would be excellent to have on home screen inbox as well. I’d strongly vote yes, though I do understand it’d be slightly tougher to design in.

@Huddie
Copy link
Collaborator Author

Huddie commented Mar 13, 2019

This PR will not include anything on the inbox but if you’re interested in that open an issue maybe with a design?

If one manually types is:open/closed, it will pass that along in the query, this does not effect the original query string.

It’s a segment control so yes, it’s to toggle between open and closed. Maybe I’ll put together a custom multi-select. Won’t be getting around to this for a week at least.

Sent with GitHawk

Copy link
Collaborator

@BasThomas BasThomas left a comment

Choose a reason for hiding this comment

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

Looking good! Really cool that you were able to whip this up so quickly 💪

// MARK: BaseListViewControllerHeaderDataSource

func headerModel(for adapter: ListSwiftAdapter) -> ListSwiftPair {
return ListSwiftPair.pair("header", { [weak self, previousSearchString] in
SearchBarSectionController(
placeholder: Constants.Strings.search,
delegate: self,
query: previousSearchString
query: previousSearchString,
items: ["Open", "Closed"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Localization? Also... what would this look like if these would be awfully long? Not sure if that would scale...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Well localization of those would never be done here, it would be done before adding to the segment control. Aka, user passes in array of strings, we localize and add to control.

@@ -29,7 +29,8 @@ IndicatorInfoProvider {
private let type: RepositoryIssuesType
private let searchKey: ListDiffable = "searchKey" as ListDiffable
private let debouncer = Debouncer()
private var previousSearchString = "is:open "
private var previousSearchString = ""
private var prefix = "is:open "
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why does the GitHub highlighter think prefix is a keyword? 🤔

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

🤷 Problem?

Copy link
Collaborator

Choose a reason for hiding this comment

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

No should be good 😊

segmentControl.addTarget(self, action: #selector(updateSegement), for: .valueChanged)

setSegmentControl()
segmentControlLeadingConstraint.deactivate()
Copy link
Collaborator

Choose a reason for hiding this comment

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

What do you need this constraint for if you deactivate it before the init returns?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Deactivated on start due to the search bar not being in editing mode. Once you start editing the constraint becomes activated

for title in items {
segmentControl.insertSegment(
withTitle: title,
at: segmentControl.numberOfSegments,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm reading this as it will always insert it at the end as the numberOfSegments is updated every time this is called?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes correct, the order in which you pass in the array is the order it will appear.

segmentControl.backgroundColor = .clear
segmentControl.tintColor = .clear

let normalFont: [AnyHashable: Any] = [
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can use [NSAttributedStringKey: Any] here instead and can then infer the type going forward. :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure thing!

}

// create a 1x1 image with this color
private func imageWithColor(color: UIColor) -> UIImage {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we have this somewhere else already by chance?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ill search

// create a 1x1 image with this color
private func imageWithColor(color: UIColor) -> UIImage {
let rect = CGRect(x: 0.0, y: 0.0, width: 1.0, height: 1.0)
UIGraphicsBeginImageContext(rect.size)
Copy link
Collaborator

Choose a reason for hiding this comment

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

You should be able to use UIGraphicsImageRenderer here instead, which removes the need to have to deal with optionals.

@BasThomas
Copy link
Collaborator

Oh and one more question... what if I want to search for both closed and open issues? That doesn't seem to be possible anymore.

@Huddie
Copy link
Collaborator Author

Huddie commented Mar 14, 2019

Oh and one more question... what if I want to search for both closed and open issues? That doesn't seem to be possible anymore.

Gonna switch out the segment control for a custom control that allows mutli selection. End result will look the same + allow multiple selection thereby open and closed or none

@nikitavoloboev
Copy link

@Huddie Have you had the time to look into adding it? Or someone else should take hold of this PR?

@Huddie
Copy link
Collaborator Author

Huddie commented Nov 9, 2019

I will try to finish this week. Otherwise I’ll let you know and it would be awesome if you or someone else picked it

Sent with GitHawk

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
💤 awaiting review Pull Request is awaiting code reviews 🚧 wip Work in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add toggle to switch between open and closed issues/PRs on repos
5 participants