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

New Bookmarks feature that syncs to iCloud #2483

Merged
merged 14 commits into from
Nov 30, 2018
Merged

New Bookmarks feature that syncs to iCloud #2483

merged 14 commits into from
Nov 30, 2018

Conversation

rnystrom
Copy link
Member

@rnystrom rnystrom commented Nov 25, 2018

  • Migrate old Bookmark models from BookmarkStore to new storage using only the GraphQL node id in iCloud's ubiquitous store
    • Take old model, build GraphQL query, find Node id for each object, append to new iCloud key store
  • Architecture that enables unit testing of VCs
  • New UI will fetch objects along w/ status from GraphQL (e.g. issue/PR open+closed+merged)

Much more to come. Putting this up now for early eyeballs.

Fixes #2379, #2377, #702, #683

@BasThomas
Copy link
Collaborator

Cool stuff!

@rnystrom
Copy link
Member Author

Slowly growing in complexity! Now RepositoryViewController loads default branch and issues-enabled when the VC loads. No need to pass that around anymore.

@rnystrom
Copy link
Member Author

simulator screen shot - iphone xs - 2018-11-27 at 21 48 53

@Huddie
Copy link
Collaborator

Huddie commented Nov 28, 2018

Looks awesome!

Sent with GitHawk

@ijm8710
Copy link

ijm8710 commented Nov 28, 2018

Yes, dynamic icons for open closed! How about sortable and having a count like bookmarks (7)

@rnystrom
Copy link
Member Author

@ijm8710 lol one thing a time

Sent with GitHawk

@ijm8710
Copy link

ijm8710 commented Nov 28, 2018

Hah what I meant to say was looks awesome...so far :)

@ijm8710
Copy link

ijm8710 commented Nov 28, 2018

That was a yes like oh yes..haha

@rnystrom rnystrom changed the title [WIP] New Bookmarks feature that syncs to iCloud New Bookmarks feature that syncs to iCloud Nov 30, 2018
@rnystrom
Copy link
Member Author

Ditching the swipe-to-delete in favor of just opening the issue/repo and un-bookmarking the item. We can bring back swipe if that becomes a huge issue, but I never used it.

@rnystrom rnystrom merged commit 49c2bec into master Nov 30, 2018
@ijm8710
Copy link

ijm8710 commented Nov 30, 2018

I always hated swipe with the concern that downswiping too fast and at the wrong angle would unintentionally remove one 🤷‍♂️

@rnystrom
Copy link
Member Author

@ijm8710 i haven’t forgot about moving either, would like to explore that. I don’t think it’d be too hard, and more valuable than swipe-delete!

Sent with GitHawk

rnystrom added a commit that referenced this pull request Nov 30, 2018
rnystrom added a commit that referenced this pull request Nov 30, 2018
* Revert "New Bookmarks feature that syncs to iCloud (#2483)"

This reverts commit 49c2bec.

* Revert "Tabbar hide fix (#2505)"

This reverts commit 460403f.
@rnystrom rnystrom deleted the bookmark-2 branch November 30, 2018 15:39
@ijm8710
Copy link

ijm8710 commented Dec 6, 2018

Did anyone’s bookmark order get all out of wack :(

@ijm8710
Copy link

ijm8710 commented Dec 8, 2018

^did this happen to you as well @rnystrom? Not a complaint; the bkmk refactor is great so far!

Just curious if the reordering was a) expected b) unlikely to occur again as I might spend a little time removing and adding them back manually to get them in the same order as before as im slightly ocd about that —obv if/once true native reordering comes if that’s still the plan that would be even easier

@Sherlouk
Copy link
Member

Sherlouk commented Dec 8, 2018

@ijm8710 @rnystrom Just updated to latest TF (was a few behind) to have a look at this and yea mine have changed order (though I'm not particularly fussed about that) but also a bunch of my bookmarks have been deleted!

Curious if that's intended too? If it helps I believe some/most were in private/2fa orgs.

Also a nice improvement is that deleted issues/orgs seem to disappear too!

@rnystrom
Copy link
Member Author

rnystrom commented Dec 8, 2018

@Sherlouk ack will take a look at the private repos part and see if there’s an early return parsing results that might have dropped it. Def my biggest concern with this is the migration deletes data.

Sent with GitHawk

@ijm8710
Copy link

ijm8710 commented Dec 8, 2018

What’s also different is new bookmarks add to bottom of list as opposed to top so I feel in addition to reshuffling it essentially reversed the order as well. Reason I brought up ordering is I felt I lost some bookmaks as well and without a total count or a recognizable order it’s hard to pinpoint that

@BasThomas
Copy link
Collaborator

Feel free to open up an issue for that @ijm8710 :)

Sent with GitHawk

@ijm8710
Copy link

ijm8710 commented Dec 8, 2018

Would you really want separate tickets for 1) lost order 2) reversed order 3) lost bookmarks?

I would feel it makes a lot more sense to streamline everything related in the same thread, especially if it’s all related to this PR?

@BasThomas
Copy link
Collaborator

Would you really want separate tickets for 1) lost order 2) reversed order 3) lost bookmarks?

Yep! This pull request has already been merged and thus not very visible. Small, easy to tackle questions can be answered and seen by more people and then are more easy to implement separately or side-by-side.

Sent with GitHawk

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.

Bookmarks show all issues in blue regardless of if issue was open or closed
5 participants