Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move starred repositories to bookmarks screen #734

Merged
merged 2 commits into from
Mar 23, 2018
Merged

Move starred repositories to bookmarks screen #734

merged 2 commits into from
Mar 23, 2018

Conversation

Tunous
Copy link
Collaborator

@Tunous Tunous commented Sep 16, 2017

stars-and-bookmarks

Related to #578, #624 and #725

@Tunous
Copy link
Collaborator Author

Tunous commented Sep 16, 2017

Myself I would prefer to have this as a separate entry in the drawer as seen in #578. But decided to create this pull request to hear your opinions.

@smichel17
Copy link
Contributor

I would prefer this implementation because, as I said in #695, I think scrollable navigation drawers are terrible ux:

Having UI elements in a consistent position means I can build muscle memory and navigate faster as I use the app more. A scrollable drawer throws this all away.

@maniac103
Copy link
Collaborator

I don't necessarily agree to the scrolling drawer argument (we have a lot of stuff to show, so we probably won't be able to avoid having a scrolling drawer - it's already scrolling in @Tunous example), but I think this implementation is fine, given that having a 'My repositories' + a 'Starred repositories' drawer item is somewhat inconsistent and potentially confusing as well.

@Tunous
Copy link
Collaborator Author

Tunous commented Sep 26, 2017

So, let's go for #725 and this + close #578 and #624?

@smichel17
Copy link
Contributor

smichel17 commented Sep 26, 2017

we have a lot of stuff to show, so we probably won't be able to avoid having a scrolling drawer - it's already scrolling in @Tunous example

Also in @Tunous's example, if Public timeline, Trending repos, and Github blog were collapsed into a single entry, it would not scroll. My phone is slightly smaller than that, so we would have to remove those 3 entries (#695), not collapse them, but it's not that far off. However, this is off-topic for this PR so I'll shut up about it now :)

@Tunous Tunous requested a review from maniac103 September 27, 2017 10:48
@maniac103
Copy link
Collaborator

I just noticed via #684 that this PR in its current state loses sort order options.

@Tunous
Copy link
Collaborator Author

Tunous commented Sep 27, 2017

Is it OK to add it as submenu similar to the one for filtering notifications? I think that will fit better than right-side menu.

@maniac103
Copy link
Collaborator

Yeah, it surely is.

@Tunous Tunous self-assigned this Sep 27, 2017
@Tunous
Copy link
Collaborator Author

Tunous commented Sep 28, 2017

Added.

@Tunous Tunous removed their assignment Sep 28, 2017
@Tunous
Copy link
Collaborator Author

Tunous commented Mar 22, 2018

@maniac103 perhaps it's time to decide on what to do with this. I think the option I mentioned here is the way to go. (I can do the merging)

@maniac103
Copy link
Collaborator

This one is fine with me; I have more issues with #725.

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

Successfully merging this pull request may close these issues.

3 participants