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

Replace places #84

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

Manan-Arora31
Copy link

Description

Replaced places section with top places section and created separate all places page

Related Issue

Fixes #81 (issue)

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)
  • This change requires a documentation update

Motivation and Context

How Has This Been Tested?

It has been tested by running on my local system.

Screenshots (if appropriate):

Checklist:

  • I have registered myself at Contrihub website.
  • My code follows the code style of this project.
  • I have commented my code, particularly in hard-to-understand areas
  • My changes generate no new warnings
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • Any dependent changes have been merged and published in downstream modules
  • I have added tests to cover my changes.
  • All new and existing tests passed.

Test Configuration:

  • Firmware version:
  • Hardware:
  • Toolchain:
  • SDK:

@Manan-Arora31
Copy link
Author

@kunal2812 I have made the PR . Please review and tell if any changes needs to be done .

Copy link
Collaborator

@kunal2812 kunal2812 left a comment

Choose a reason for hiding this comment

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

  • Places are not visible at all on the home page even though they are present.
  • On the 'All Places' page, I am unable to see full information about the place. When I click on the place, a modal should open, just like it did on the home page

Code is poorly tested. Please check #81 to see what is required.

@Manan-Arora31
Copy link
Author

@kunal2812 Sir I have fetched the places for top places from review ratings model, so only the places which are rated will be visible.

@Manan-Arora31
Copy link
Author

Otherwise on what basis will we display the top places ?

@kunal2812
Copy link
Collaborator

If we do not have enough rated places we can populate the "Top Places" section with places without ratings. The key point is that it should not be empty if we have places available.

@Manan-Arora31
Copy link
Author

@kunal2812 Should I make it same as the previous all places section in this case?

@kunal2812
Copy link
Collaborator

Suppose there are 4 places - Place1: Rating 4, Place2: Rating 5, Place3: Not Rated, Place4: Not Rated.

So in the top section places the order should be - Place2 > Place1 > (Place3 = Place4)

@Manan-Arora31
Copy link
Author

Okay I will implement this

@Manan-Arora31
Copy link
Author

@kunal2812 I have made the required changes . Please see to it .

@kunal2812
Copy link
Collaborator

image

Check the modal on "Home" page

image

This is your modal on "All Places" page

Fix it properly.

@Manan-Arora31
Copy link
Author

@kunal2812 I have corrected the modal . Please see it

@Manan-Arora31
Copy link
Author

@kunal2812 Sir I have implemented all the features now . Please check if anything else needs to be done .

@kunal2812
Copy link
Collaborator

kunal2812 commented Oct 25, 2023

image

This functionality is not working in "All Places" page. Also make the cards in "All Places" page look like as they are in "Home" page.
There is no weather information in the modal as well.

image

Uncomment these lines for weather info. Someone pushed without uncommenting them.

@Manan-Arora31
Copy link
Author

@kunal2812 Sir I have corrected the frontend of the cards and uncommented the lines . I tried hard to implement the functionality but couldn't do it .

@kunal2812
Copy link
Collaborator

"Check Safety" functionality is already implemented. You just have to use it. Refer to "Home" page to see how it's done. Try to complete it.

@Manan-Arora31
Copy link
Author

@kunal2812 Sir I tried a lot to implement the functionality . But I am not able to do it.

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

Successfully merging this pull request may close these issues.

Replace "Places" Section with "Top Places" and Create Separate "All Places" Page
2 participants