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

Native Create Issue #311

Merged
merged 14 commits into from Sep 25, 2017
Merged

Native Create Issue #311

merged 14 commits into from Sep 25, 2017

Conversation

Sherlouk
Copy link
Member

@Sherlouk Sherlouk commented Sep 15, 2017

Closes #288
Closes #350

  • Created view controller to natively create an issue supporting a title and body text
  • Pass through the correct GithubClient to Settings VC (necessary for correct authentication)
  • Updated "Report a bug" tool to use issue creator (currently using a test repo)
  • On a successful creation, the user is segued to the issues view controller (replacing new issue, so they can't go back to the form!)

Example output: rnystromtest/Githawk-Playground#2
Used API: https://developer.github.com/v3/issues/#create-an-issue

@Sherlouk
Copy link
Member Author

Sherlouk commented Sep 16, 2017

  • Added new issue button to all repositories that have issue enabled
  • Added markdown/action bar
  • Added a separate preview button (Wanted to put the actual preview in the same box but turn off editing, but this was too difficult so I kept it simple and pushed it out the same VC we use at comment level)
  • Fixed issues on tablet so things appear in the correct panel
  • Fully localized now
  • Keyboard will now update the insets in the scroll view
  • Pressing "return" when in the title field, will take you to the body field
  • Body field will expand as new lines are added

image

Still looks like 💩 - If anyone wants to give a hand making it look nice I would appreciate it 😅

@Sherlouk
Copy link
Member Author

I'd say for an initial implementation I'm happy with it now - Obviously we can add new functionality down the line for adding labels, assignees etc (assuming you have push permissions) but I think this is good?

@BasThomas
Copy link
Collaborator

Regarding design: a few years back I did something similar regarding native reporting, and think that looked pretty nice. Obviously a bit different in comparison with creating an issue, but design wise you get the idea.

screen shot 2017-09-16 at 17 13 32

@Sherlouk
Copy link
Member Author

@BasThomas You're more than welcome to branch off and try match that!

It looks a look more modular, and similar to Eureka just couldn't see a nice way of doing it without ballooning hence with the simpler scroll view

@rnystrom
Copy link
Member

rnystrom commented Sep 17, 2017

Will give this a deeper look soon, def need to massage the design. I personally prefer the look of the stuff @BasThomas posted: grouped UITableView style, tho it might be overkill for title+body. I'm not sure yet the best way to incorporate the editing tools. Input accessory view is prob the best bet for this.

@Sherlouk
Copy link
Member Author

Sherlouk commented Sep 17, 2017

Design has never been a hotspot of mine, and editing tools are definitely up for discussion (I basically just copied what we had already to minimise on work!)

This does look quite nice though: http://1writerapp.com/
In essence a custom keyboard

If someone wants to pick up the design then I'd be appreciative - otherwise I can try and copy BasThomas' design above

@BasThomas
Copy link
Collaborator

Probably also want to add this option for repos you’re browsing via search. Will create a separate issue.

@Sherlouk
Copy link
Member Author

This is available via search and for report a bug - am I missing something @BasThomas

Sent with GitHawk

@rnystrom rnystrom mentioned this pull request Sep 21, 2017
@BasThomas
Copy link
Collaborator

BasThomas commented Sep 22, 2017

I don’t get it. 😬 What are you referring to?

I am expecting, if you look at the screenshot below, to also have an option to “Open an Issue”

ebb7656d-6db1-4f47-a878-d725e5d93988

@@ -48,6 +48,30 @@ PrimaryViewController {
feed.viewDidLoad()
feed.adapter.dataSource = self
title = "\(repo.owner)/\(repo.name)"

if repo.hasIssuesEnabled {
navigationItem.rightBarButtonItem = UIBarButtonItem(
Copy link
Member Author

Choose a reason for hiding this comment

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

@BasThomas You mean like this code here 😉

I know I need to move it to the action sheet, but the logic is all here to have a "New Issue" button for the repo view controller - Just don't merge master into this branch 😂

# Conflicts:
#	Classes/Repository/RepositoryViewController.swift
#	Resources/Info.plist
@Sherlouk
Copy link
Member Author

@rnystrom Just updated with changes in master so the "Create Issue" is now in the overflow rather than it's own button

In terms of design, https://github.com/xmartlabs/Eureka would give us a basic form with all the structure done for us -- Would this be a solution you're okay with?

@rnystrom
Copy link
Member

Is that project gonna be overkill tho? Will we use it for more stuff in the future? Nervous about taking another dependency for a simple design.

Sent with GitHawk

@Sherlouk
Copy link
Member Author

Further down the line I'm sure the form will get more complicated with files/images, labels, assignees, etc

Other forms include add/edit projects, maybe even editing repos

I'll give it a quick go now without Eureka and see how it ends up

@Sherlouk
Copy link
Member Author

@rnystrom

image

You mentioned before that we need to come up with a better way of doing markdown so I left the buttons out of this design. Is this what you had in mind?

@Sherlouk
Copy link
Member Author

Sherlouk commented Sep 22, 2017

cc @BasThomas ^^ also

@rnystrom
Copy link
Member

Ya that looks pretty great, nice and simple. We should be able to add the markdown actions as the input accessory view, right? @Sherlouk can you confirm that's still possible here?

@Sherlouk
Copy link
Member Author

Sherlouk commented Sep 23, 2017

It should still be possible yea (it’s just a text view!) will look into it

Sent with GitHawk

# Conflicts:
#	Classes/Repository/RepositoryViewController.swift
#	Resources/Info.plist
@Sherlouk
Copy link
Member Author

image

Added markdown controls ^^

@rnystrom @BasThomas

@rnystrom
Copy link
Member

Yessss!

@Sherlouk
Copy link
Member Author

@rnystrom We waiting on anything to merge this now? 😄

@Sherlouk Sherlouk changed the title WIP: Native Create Issue Native Create Issue Sep 23, 2017
@BasThomas
Copy link
Collaborator

Nice!

@rnystrom
Copy link
Member

@Sherlouk don’t think so! I’ll give the code a quick look, but I also don’t mind refactoring anything on my own. Will merge today!

Sent with GitHawk

@Sherlouk
Copy link
Member Author

@rnystrom Hit me up when ready to merge and I can deal with the conflicts

@rnystrom
Copy link
Member

@Sherlouk give it a go and DM me on Twitter so I can merge ASAP to avoid more conflicts!

@Sherlouk
Copy link
Member Author

@rnystrom Picking this up now, will be 5 minutes

# Conflicts:
#	Classes/Repository/RepositoryViewController.swift
#	Classes/Settings/SettingsViewController.swift
#	Resources/Info.plist
@Sherlouk
Copy link
Member Author

All good!

@rnystrom rnystrom merged commit f40557e into master Sep 25, 2017
@Sherlouk Sherlouk deleted the newIssue branch September 26, 2017 08:06
@weyert
Copy link
Contributor

weyert commented Sep 27, 2017

May I ask why you have chosen a UITableView for this and not a UICollectionView or let's go wild and a UIStackView? I am still trying to find out when to use what. I really like UICollectionView myself as it's more powerful with it custom layout's support.

@Sherlouk
Copy link
Member Author

Originally it was a UIScrollView with a couple elements in, ultimately I would've been happy with a bit of auto layout magic but crucially we still needed to support all the screen sizes so scrolling was a must

In the end it came down to simplicity and "out of the box" design. A grouped UITableView gives us a consistent design not only across the app but also familiarity across iOS. Using static cells makes it super easy to implement and all values can be accessed without the need for a maze of delegates and hierarchy calls.

In this situation there was very little flexibility required, though when we do get the point of adding additional fields, such as labels, this may have to change.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants