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

Show discard or keep alert after tapping outside of the PR reviewers,… #2633

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

julienbodet
Copy link

… assignees, milestones, labels modal

This my first contribution to GitHawk :) Thanks for the awesome project!

I implemented the feature from #2582. Now after tapping outside the modal for PR reviewers, assignees, milestone or labels, an alert appears asking to keep or discard the changes, if any.

@Huddie Huddie added the 💤 awaiting review Pull Request is awaiting code reviews label Feb 13, 2019
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.

Some comments, but looks good. Can we maybe add some tests? 😇
Let me know if there's anything I can help with. Congrats on the contribution!

@@ -323,10 +335,22 @@ final class IssueManagingContextController: NSObject, ContextMenuDelegate {

let selected = controller.selected
guard controller.selectionChanged(newValues: selected) else { return }

if controller.wasDismissedByDone {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we put this logic in a function, as it seems to be repeated from lines 312-320?

Classes/Issues/IssueManagingContextController.swift Outdated Show resolved Hide resolved
Classes/Labels/LabelsViewController.swift Outdated Show resolved Hide resolved
Classes/Issues/IssueManagingContextController.swift Outdated Show resolved Hide resolved
@@ -30,6 +30,8 @@ PeopleSectionControllerDelegate {
private var owner: String
private var repo: String

var wasDismissedByDone: Bool = false
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this variable be included in the delegate maybe? What do you think?

Copy link
Author

@julienbodet julienbodet Feb 23, 2019

Choose a reason for hiding this comment

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

Are you talking about ContextMenuDelegate? Maybe we can modify its functions signatures to add this flag:

public protocol ContextMenuDelegate: class {
    func contextMenuWillDismiss(viewController: UIViewController, animated: Bool, doneTapped: Bool)
    func contextMenuDidDismiss(viewController: UIViewController, animated: Bool, doneTapped: Bool)
}

Copy link
Collaborator

@BasThomas BasThomas Feb 23, 2019

Choose a reason for hiding this comment

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

Yes, that sounds good! Would maybe make it didTapDone (or maybe even viaButton: Button or similar, do be able to differentiate?)

A bit tricky as the ContextMenu is a separate pod and will probably not always contain a Done.

@BasThomas BasThomas added 😴 awaiting changes Changes requested, waiting on author to update and removed 💤 awaiting review Pull Request is awaiting code reviews labels Feb 22, 2019
@Huddie
Copy link
Collaborator

Huddie commented Mar 13, 2019

@julienbodet still on this?

Sent with GitHawk

@julienbodet
Copy link
Author

@julienbodet still on this? ...

@Huddie yep still on it. I addressed most of comments but I didn’t push yet.

Sent with GitHawk

@BasThomas BasThomas added 💤 awaiting review Pull Request is awaiting code reviews and removed 😴 awaiting changes Changes requested, waiting on author to update labels Mar 20, 2019
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants