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

Adds options to remove bookmark #696

Merged
merged 5 commits into from
Oct 25, 2017
Merged

Adds options to remove bookmark #696

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
784d77e
[ADD] options to remove bookmark
rizwankce Oct 23, 2017
65e96ea
Merge branch 'master' into feature/remove-bookmark
rnystrom Oct 23, 2017
69c2c84
Merge branch 'master' into feature/remove-bookmark
rizwankce Oct 24, 2017
a099969
[ADD] Move bookmark actions to bottom of action sheet
rizwankce Oct 24, 2017
678e2bf
Merge branch 'master' into feature/remove-bookmark
rnystrom Oct 25, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions Classes/Bookmark/BookmarkStore.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,10 @@ final class BookmarksStore {
archive()
}

func contains(bookmark: BookmarkModel) -> Bool {
return _bookmarks.contains(bookmark)
}

func clear() {
_bookmarks.removeAll()
archive()
Expand Down
14 changes: 12 additions & 2 deletions Classes/Utility/AlertAction.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,8 +143,18 @@ struct AlertAction {
}

static func bookmark(_ bookmark: BookmarkModel) -> UIAlertAction {
return UIAlertAction(title: NSLocalizedString("Add To Bookmark", comment: ""), style: .default) { _ in
BookmarksStore.shared.add(bookmark: bookmark)
let isNewBookmark = !BookmarksStore.shared.contains(bookmark: bookmark)
let title = isNewBookmark ? Constants.Strings.bookmark : Constants.Strings.removeBookmark
return UIAlertAction(
title: title,
style: isNewBookmark ? .default : .destructive
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

"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

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 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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

On the discussion regarding "destructive": #532

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You mean "Remove bookmark" action to the bottom of the action sheet?
Yes, I'm down with that.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yep, the add + remove bookmark :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Moved bookmark actions to bottom of the action sheet. 👍

) { _ in
if isNewBookmark {
BookmarksStore.shared.add(bookmark: bookmark)
}
else {
BookmarksStore.shared.remove(bookmark: bookmark)
}
}
}
}
1 change: 1 addition & 0 deletions Classes/Views/Constants.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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: "")
Copy link
Member

Choose a reason for hiding this comment

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

Is this used anywhere else?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Nope. I just don't like to have random string in AlertActions

Copy link
Collaborator

@BasThomas BasThomas Oct 24, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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?

}
}
8 changes: 8 additions & 0 deletions FreetimeTests/BookmarkStoreTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,4 +56,12 @@ final class BookmarkStoreTests: XCTestCase {

XCTAssert(BookmarksStore.shared.bookmarks.count == 0)
}

func test_containsBookmark() {
let b1 = BookmarkModel(type: .repo, name: "GitHawk", owner: "rizwankce")

BookmarksStore.shared.add(bookmark: b1)
Copy link
Member

Choose a reason for hiding this comment

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

Gotta get rid of this singleton 😉

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 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.


XCTAssert(BookmarksStore.shared.contains(bookmark: b1))
}
}