Adds options to remove bookmark #696
Conversation
Classes/Views/Constants.swift
Outdated
| @@ -42,5 +42,6 @@ enum Constants { | |||
| static let inbox = NSLocalizedString("Inbox", comment: "") | |||
| static let upload = NSLocalizedString("Upload", comment: "") | |||
| static let bookmark = NSLocalizedString("Bookmark", comment: "") | |||
| static let removeBookmark = NSLocalizedString("Remove Bookmark", comment: "") | |||
There was a problem hiding this comment.
Nope. I just don't like to have random string in AlertActions
There was a problem hiding this comment.
Although I get it, I think it would be more in line to keep single-use strings "hardcoded" as to not clutter the Constants even more. @rnystrom?
| func test_containsBookmark() { | ||
| let b1 = BookmarkModel(type: .repo, name: "GitHawk", owner: "rizwankce") | ||
|
|
||
| BookmarksStore.shared.add(bookmark: b1) |
There was a problem hiding this comment.
Gotta get rid of this singleton 😉
There was a problem hiding this comment.
Yes Yes. I just don't want to give a big PR with all changes. Wanted to give a separate/small PR after I fix the initial round of bug fixes. So it would be easy for us to review/conflicts free PR.
| let title = isNewBookmark ? Constants.Strings.bookmark : Constants.Strings.removeBookmark | ||
| return UIAlertAction( | ||
| title: title, | ||
| style: isNewBookmark ? .default : .destructive |
There was a problem hiding this comment.
Actually can we just make this .default? Seeing the destructive "red" next to other actions like "Close" and "Lock" is a little weird b/c this doesn't carry nearly the same weight. cc @BasThomas for thoughts
There was a problem hiding this comment.
I think it would make sense, but I think we’d like to move the action away from lock/close. Also, both Bookmark and Remove Bookmark have “side effects”, so (as we’re using it right now) both actions should be “destructive”.
I feel like it’s time to explore something completely different than the (now very long) action sheets...
There was a problem hiding this comment.
"Remove bookmark" does not carry the same weight as other destructive buttons. But IMO it would avoid accidentally tapping.
From Apple doc
case destructive
Apply a style that indicates the action might change or delete data.
Also, I do agree with @BasThomas. Action sheet is very long now. After 1.12.0 we should rethink and adopt a better way to handle this action sheet for sure
There was a problem hiding this comment.
What do you think about moving the action (more) to the bottom? I think it doesn't make sense that it is next (and in this case even above) some probably more used, destructive actions.
There was a problem hiding this comment.
On the discussion regarding "destructive": #532
There was a problem hiding this comment.
You mean "Remove bookmark" action to the bottom of the action sheet?
Yes, I'm down with that.
There was a problem hiding this comment.
Yep, the add + remove bookmark :)
There was a problem hiding this comment.
Moved bookmark actions to bottom of the action sheet. 👍
* master: Removed cell dividers, added bg color (GitHawkApp#690) Fix capitalization for short words (GitHawkApp#680) Rename "Add to Bookmark" to simply "Bookmark" (GitHawkApp#681) Standardized sign-in/out copy (GitHawkApp#689) Preventing the search race condition instead of responding to it (GitHawkApp#691) [FIX] scroll to top for bookmarks screen (GitHawkApp#693) Remove erroneous double localization Some general text improvements # Conflicts: # Classes/Utility/AlertAction.swift # Classes/Views/Constants.swift
|
re: action sheet growing, I'm brainstorming ways to move all edit/action functionality into some sort collapsed feature set. Same w/ editing labels/milestones/etc. Would be nice to put it all in one place that is tucked away. |
Closes #684
Probably overrides #681
BookmarkorRemove Bookmarktitle