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

Display nearby books in Explore Nearby Screen Iplemented #140

Merged
merged 11 commits into from
Oct 24, 2021

Conversation

kshitij5
Copy link
Contributor

@kshitij5 kshitij5 commented Oct 9, 2021

Description

This PR tries to implement the Books Explore section #94 . Below is the screenshot. Will be happy to refactor code if necessary!

Screenshot_1633787397

Fixes #94

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

  • My code follows the code of conduct of this project
  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • My changes generate no new warnings

@Mohitmadhav
Copy link
Member

Mohitmadhav commented Oct 10, 2021

How did you get the books more than 20 km?
The books to be displayed have to be added by the user through Add your book through their profile page.
Because we also need to find the users who added the book

@kshitij5
Copy link
Contributor Author

Yes, I am fetching those books only that are added by any particular user.
The flow of the algorithm is to first get all the other users' detail and using their lat/long value to find how far they are. Then basically filtering them accordingly, before getting and displaying books added by them!

Copy link
Member

@Mohitmadhav Mohitmadhav left a comment

Choose a reason for hiding this comment

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

@kshitij5 dont include the owned books of the user who is running the app in the Explore nearby screen.

Rest everything is fine but there was another glitch for which I'll create a new issue if you want, or you can make changes in this PR only.

@kshitij5
Copy link
Contributor Author

@Mohitmadhav I already thought about that situation and no books listed by the user who is running the app are being shown in explore section.
Regarding the glitch, yes, you can create a new issue!

@Mohitmadhav
Copy link
Member

Yeah @kshitij5, but actually the issue creation would be for Navigation to the book description page in Explore screen not the play store link (That play store link navigation should only be for the dashboard page books)

And please don't waste too much time on lint
I already have another issue created for that. See #99.
And also #79. So don't take too much time on those

@kshitij5
Copy link
Contributor Author

Understood ! @Mohitmadhav, I actually completely overlooked the navigation part because the Figma UI suggests that both books from the dashboard and Explore section follow the same navigation! So, I thought once the Discover Now section is implemented, explore navigation will also be implemented.

Anyway, what should I do next?

Should I create another book card widget, which irrespective of if the book has complete info or not...will always navigate to the book description page?

@Mohitmadhav
Copy link
Member

No no @kshitij5 , actually the thing is...
We want to navigate from

  • Dashboard - > Playstore, and
  • Explore Nearby -> Book Description page,

so changes could be made in the existing BookCard Widget.

@kshitij5
Copy link
Contributor Author

Okay! I'll make the necessary changes tomorrow.

@Mohitmadhav
Copy link
Member

@kshitij5 pull the latest changes using
git pull upstream main before pushing your changes
And then notify me here.

@kshitij5
Copy link
Contributor Author

Hi, as I was looking to make changes to the BookCard widget to get the required behavior I couldn't think of any way to navigate that way!

One way to achieve that behavior could be to stop storing infolink for the book since we are reusing the same widget both in the dashboard and Explore Books screen there is no way to know in which screen we need to navigate!

Please leave me with some hints if you know how to achieve that!

@Mohitmadhav
Copy link
Member

Right @kshitij5 so what you can do is...
Navigate everything to Book Description Screen for now
Then we'll have to resolve #79 after which some changes will be made in the public profile for which I'll create another issue.

@HARSHBHUDOLIA
Copy link
Collaborator

For now I also can think of only that way which you did, one way to improve the algorithm is to store which city the user is in addition to its location, this can be helpful as we then only have to calculate distance of those in same city, we can also customise it to add blocks/sector in cities.

The Geolocation package/api gives a detailed address based on location, so if we can store all the different values like city/state/pin number/street in user in different fields then we can do a better search, we can start with Pin Number, which is generally associated with nearest post office to limit the calculations.

I hope what I am saying is understandable

@HARSHBHUDOLIA
Copy link
Collaborator

This way we can reduce time complexity and make it more scalable

@kshitij5
Copy link
Contributor Author

kshitij5 commented Oct 16, 2021

@Mohitmadhav I have made changes so that everything gets navigated to Book Description Screen for now!

@HARSHBHUDOLIA yes, for that we might need to refactor the users model again! Or we might even completely change the structure of the collection. Instead of nesting books under user, we can create a separate books collection with userId as a foreign key having other details as you mentioned!

@Mohitmadhav
Copy link
Member

Mohitmadhav commented Oct 19, 2021

@kshitij5 see #92, changes can be made there.
I think you should create a separate issue for that.
I will assign it after this PR
Also, resolve these merge conflicts...It's now migrated to null safety

@kshitij5
Copy link
Contributor Author

@Mohitmadhav I have solved the merge conflict and as for the efficient book fetching API, I'll create another issue, where we can address and solve that!

@kshitij5
Copy link
Contributor Author

@Mohitmadhav @HARSHBHUDOLIA I was thinking if is it possible to get point B's latlong (in range) if we know the point A's latlong and distance between Point A and Point B? If it's possible then fetching it will become significantly easier!

@HARSHBHUDOLIA
Copy link
Collaborator

HARSHBHUDOLIA commented Oct 20, 2021 via email

@kshitij5
Copy link
Contributor Author

@Mohitmadhav Hi, I don't think I can improve the fetching algorithm anymore to the best of my knowledge! You can merge this PR if it satisfies the requirement!

@Mohitmadhav
Copy link
Member

Right @kshitij5 so what you can do is... Navigate everything to Book Description Screen for now Then we'll have to resolve #79 after which some changes will be made in the public profile for which I'll create another issue.

@kshitij5 I implemented it for now...
But there'll be some problems which we might face.
I'll create issues for those too

@Mohitmadhav Mohitmadhav added the hacktoberfest-accepted Approval of PR in Hacktober fest label Oct 24, 2021
@Mohitmadhav Mohitmadhav merged commit 24ba798 into Project-Easter:main Oct 24, 2021
@kshitij5
Copy link
Contributor Author

@Mohitmadhav yes can create issue for that and I'll try to work on them too!

@Mohitmadhav
Copy link
Member

Yeah sure @kshitij5 ...
And also, it's a long term project (not solely meant just for Hacktober)
So I would appreciate it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hacktoberfest-accepted Approval of PR in Hacktober fest
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Display nearby books in Explore Nearby Screen
3 participants