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

Allows for single and double tap on each post #16

Merged
merged 5 commits into from
Jun 8, 2020

Conversation

banaslee
Copy link
Contributor

@banaslee banaslee commented Jun 3, 2020

Fixes #9

Quite a lot of changes in order to achieve this:

  • had to activate the navigation link programmatically in order to capture single and double tap
  • NavigationLink activated based on new view state for selected post tag
  • moved the RequestView to the ContentView so the selected post wouldn't reload the whole PostList and repeat the request
  • PostList now initialised with Listing object
  • setting the contentShape of the PostView in the list in order to capture hits in the whole area, otherwise it would land on the NavigationLink behind
  • adding two tap gestures in order of precedence
  • set the default frame for the new spinner to guarantee a minimum size while loading

The only secondary effect from this solution I wasn't able to fix is that now cells don't get a selected state like before. Hopefully someone knows how to fix that, otherwise can be a good bug to follow up with.

Quite a lot of changes in order to achieve this:
- had to activate the navigation link programmatically in order to capture single and double tap
- NavigationLink activated based on new view state for selected post tag
- moved the RequestView to the ContentView so the selected post wouldn't reload the whole PostList and retriever the request
- PostList now initialised with Listing object
- setting the contentShape of the PostView in the list in order to capture hits in the whole area, otherwise it would land on the NavigationLink behind
- adding two tap gestures in order of precedence
- set the default frame for the new spinner to guarantee a minimum size while loading

The only secondary effect from this solution I wasn't able to fix is that now cells don't get a selected state like before. I accept suggestions.
@banaslee banaslee changed the title Fixing https://github.com/carson-katri/reddit-swiftui/issues/9 Allows for single and double tap on each post Jun 3, 2020
@carson-katri
Copy link
Owner

Maybe for now fake the selection with a .background(Color.blue)?

@banaslee
Copy link
Contributor Author

banaslee commented Jun 6, 2020

Got selection state but unfortunately is not blue like before with NavigationLink.
Quite lost actually. Selection seems to be different than highlighting but I'm now starting to understand why people complain about SwiftUI documentation :)

banaslee added 2 commits June 7, 2020 16:25
…st id init parameters;

Using a computed property for the list and navigation link selections;
Listing now has a computed property to return an array of Posts.
Made the section non-collapsible
@banaslee
Copy link
Contributor Author

banaslee commented Jun 7, 2020

Couldn't get a blue selection after many tries but there's a strange bug:

  1. select a post
  2. see the post now has a grey selected background
  3. tap section header
  4. see the selected post now has a blue selected background
  5. all consecutive selections will have a blue background as well

I'm throwing the towel on this one :)

@carson-katri
Copy link
Owner

carson-katri commented Jun 8, 2020

This looks really good, but there is one issue 😂 (besides the one you mentioned, but I think it's related).

If you click on the edge (outside the text or images), it gives a blue highlight but doesn't actually show the page. Could you push the clickable area all the way up to the edge so they can't select that?

Hopefully Apple releases some fixes for this stuff in a few weeks with macOS 10.16

@banaslee
Copy link
Contributor Author

banaslee commented Jun 8, 2020

I’ll give it a try.

But if these are the only issues I propose it’s merged and new issues get opened. Can become good issues for first contributors.

@carson-katri
Copy link
Owner

Ok, I’ll merge now. Great work

@carson-katri carson-katri merged commit 50f389d into carson-katri:master Jun 8, 2020
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.

[macOS] Clicking on the post list text, the NavigationLink isn't triggered
2 participants