Skip to content

feat: sort by rating#800

Closed
michaeliuedu wants to merge 6 commits intoScottyLabs:mainfrom
michaeliuedu:main
Closed

feat: sort by rating#800
michaeliuedu wants to merge 6 commits intoScottyLabs:mainfrom
michaeliuedu:main

Conversation

@michaeliuedu
Copy link
Copy Markdown
Member

give user choice to sort by rating (highest to lowest or lowest to highest) or by the default times. i added a placeholder dropdown for this. also commits are kinda messy sorry

Copilot AI review requested due to automatic review settings March 24, 2026 20:26
@vercel
Copy link
Copy Markdown

vercel bot commented Mar 24, 2026

@michaeliuedu is attempting to deploy a commit to the ScottyLabs Team on Vercel.

A member of the Team first needs to authorize it.

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Adds a user-facing sort control to the list view (time vs rating) by fetching per-location review summaries and threading rating data through the location model, alongside some related UI/API cleanup (report endpoint + banner refactor).

Changes:

  • Add a sort dropdown on the List page and support sorting cards by rating (asc/desc) or existing time-based ordering.
  • Fetch review summary data per location and attach averageRating/ratingCount onto ILocation_Full.
  • Refactor the top banner into a reusable GeneralBanner and update the report API schema/usage.

Reviewed changes

Copilot reviewed 13 out of 21 changed files in this pull request and generated 8 comments.

Show a summary per file
File Description
src/util/storage.ts Minor formatting fix.
src/util/queryLocations.ts Import formatting only.
src/types/locationTypes.ts Add rating data types and extend ILocation_Full.
src/types/api.d.ts Update generated API types (report payload, new reports endpoint).
src/pages/ListPage.tsx Add sort control + click-to-close-drawer behavior; refactor into ListBox.
src/index.css Add --red-700 color token.
src/components/banners/GeneralBanner.tsx Generalize banner into reusable component with configurable text + storage key.
src/components/banners/GeneralBanner.module.css Minor CSS tweak (top: 0).
src/components/SelectSort.tsx New sort <select> control.
src/components/SelectLocation.tsx Formatting-only change.
src/components/EateryCardHeader.tsx Update report UI copy and align POST body with updated API.
src/components/EateryCardHeader.module.css Update report button colors/hover token.
src/components/EateryCardGrid.tsx Implement rating-based sort option.
src/components/EateryCardGrid.module.css Formatting-only change.
src/components/EateryCardContent.tsx Formatting-only change.
src/components/EateryCardContent.module.css Formatting-only change.
src/components/EateryCard.tsx Formatting-only change.
src/components/EateryCard.module.css Formatting-only change.
src/App.tsx Fetch ratings via React Query useQueries and pass into full location objects; swap to GeneralBanner.
src/App.css Fix mobile layout rules by moving them into a top-level media query.
api.yaml Update /report schema + add /v2/locations/{locationId}/reports endpoint.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

toast.error('Failed to submit report!');
// eslint-disable-next-line no-console
console.error(error);
toast.error(`Failed to submit report: ${error}`);
Copy link

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

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

toast.error(Failed to submit report: ${error}) will usually render as [object Object] (or otherwise leak unstructured details) because error is not guaranteed to be a user-friendly string. Keep the toast message human-readable (e.g., generic failure or error.message when available) and rely on console.error/logging for the full error object.

Suggested change
toast.error(`Failed to submit report: ${error}`);
const errorMessage =
typeof error === 'string'
? error
: (error as { message?: string })?.message ||
'An unexpected error occurred while submitting your report.';
toast.error(`Failed to submit report: ${errorMessage}`);

Copilot uses AI. Check for mistakes.
Comment on lines +538 to +542
- type: Date
- format: date-time
type: string
- format: date
type: string
Copy link

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

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

type: Date is not a valid OpenAPI Schema Object type value (OpenAPI only supports string/number/integer/boolean/object/array/null). Since you already allow date-time/date strings and numbers, removing the type: Date branch (or converting it to a string with an appropriate format) will keep the spec/tooling valid.

Suggested change
- type: Date
- format: date-time
type: string
- format: date
type: string
- type: string
format: date-time
- type: string
format: date

Copilot uses AI. Check for mistakes.
Comment on lines +1 to +2
/* eslint-disable jsx-a11y/click-events-have-key-events */
/* eslint-disable jsx-a11y/no-static-element-interactions */
Copy link

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

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

Disabling jsx-a11y/no-static-element-interactions and click-events-have-key-events for the entire file hides legitimate accessibility regressions. Prefer addressing the specific interaction (e.g., move the outside-click handler to a dedicated overlay/button, or add proper role/tabIndex + keyboard handling) and scope any lint disable to the smallest possible block.

Copilot uses AI. Check for mistakes.
michaeliuedu and others added 2 commits March 24, 2026 17:27
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@Arom1a
Copy link
Copy Markdown
Contributor

Arom1a commented Mar 24, 2026

Thanks for your work on this! Could you push your branch directly to this repo instead your forked repo? This way the vercel will automatically deploy the branch without extra authorization issue.

@michaeliuedu
Copy link
Copy Markdown
Member Author

Thanks for your work on this! Could you push your branch directly to this repo instead your forked repo? This way the vercel will automatically deploy the branch without extra authorization issue.

thanks! i tried pushing my branch directly upstream, but i have a 403 permission error, so i dont think i have write access. is there any way i could receive access to do so?

@Arom1a
Copy link
Copy Markdown
Contributor

Arom1a commented Mar 24, 2026

Which specific command have you run? Could you try this one: git push -u upstream sort-by-rating? If it still gives u an error, I will go ask Eric to set your permissions in this repo.

@Arom1a
Copy link
Copy Markdown
Contributor

Arom1a commented Mar 24, 2026

After you have successfully pushed to this repo, you can close this pr and open a new one from the branch you pushed to this repo.

@michaeliuedu
Copy link
Copy Markdown
Member Author

I tried git push -u upstream sort-by-rating, and I’m still getting a 403 permission error: Permission to ScottyLabs/cmueats.git denied to michaeliuedu. So it looks like I still don’t have push access to the repo.

@Arom1a
Copy link
Copy Markdown
Contributor

Arom1a commented Mar 25, 2026

Got it

@michaeliuedu
Copy link
Copy Markdown
Member Author

okay i seem to have access and pushed it

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