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.

Sign up for GitHub

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

Jump to bottom

Issue 7 Leaderboard Page #8

Merged
merged 6 commits into from
Dec 7, 2024
Merged

Issue 7 Leaderboard Page #8

merged 6 commits into from
Dec 7, 2024

Conversation

loklokyx
Copy link
Contributor

@loklokyx loklokyx commented Nov 26, 2024 •
edited
Loading

Change Summary

server:

  • initial app directory, including migrations folder and base files for the route /api/app/leaderboard/

client:

  • created basic UI with a simple table, dropdown, and search
  • implemented search functionality with dropdown and input options
  • added AJAX to retrieve BE sample data and search functionality in Leaderboard page /admin/leaderboard

Change Form

Fill this up (NA if not available). If a certain criteria is not met, can you please give a reason.

  • [Y] The pull request title has an issue number
  • [Y] The change works by "Smoke testing" or quick testing
  • [NA] The change has tests
  • [NA] The change has documentation

Other Information

server:

  • temporarily use sample data before model creation for serialization
  • permission validation is currently not implemented

client:

  • pending layout implementation to merge Leaderboard body content

Related issue

@loklokyx
create Leaderboard page
fdda093 
server:
- initial `app` directory, including migrations and base files for the route `/api/app/leaderboard/`.
- temporarily use sample data before model creation for serialization
- permission validation ais currently not implemented

client:
- created basic UI with a simple `table`, `dropdown`, and `search`
- implemented search functionality with dropdown and input options
- added AJAX to retrieve BE sample data and search functionality in Leaderboard page `/admin/leaderboard`
- Pending: layout implementation to merge Leaderboard body content
@loklokyx loklokyx added backend Task must have a back end issue enhancement New feature or request frontend Task must have a front end issue labels Nov 26, 2024
@loklokyx loklokyx self-assigned this Nov 26, 2024
@loklokyx
Copy link
Contributor Author

image
This is the current UI of the Table and Search Form

@loklokyx
refactor and clean up client side
2f8c3cb 
Refactor: replace `useEffect` with React Query for state management
- removed manual useEffect and useState logic
- simplified state handling with `useQuery`

Refactor: update `Table` component to follow `shadcn/ui` standard
- improved structure and styling for consistency and maintainability
yunho7687
yunho7687 requested changes Dec 2, 2024
View reviewed changes
Copy link
Member

@yunho7687 yunho7687 left a comment

Choose a reason for hiding this comment

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

Amazing works!
Need to mind that we don't need a new app named "app". there's a app named app in penni repo cuz people misunderstood the objective of the django server, which should be an api server and had already been created as "api". you can take this repo as ref: https://github.com/codersforcauses/csf/tree/main/server

@yunho7687
Copy link
Member

You can use code below to create a sub-app in Django:

cd api
django-admin startapp sub-app-name-such-as-auth

and don't forget to install the app in INSTALLED_APPS.

Here's a ref to that
https://stackoverflow.com/questions/2862084/how-do-i-create-sub-applications-in-django

@loklokyx
refactor: reorganise structure, remove redundant app
1c8a5f1 
- remove the unnecessary `app` directory as per feedback
- rename `admin` folder to `manager` to avoid conflict with Django's default admin namespace
- update all relevant file paths and imports to reflect the new directory name
- follow the sub-app approach by creating a sub-app within `api` using `django-admin startapp`
- integrate the new sub-app in `INSTALLED_APPS` to ensure proper functionality
@loklokyx
Copy link
Contributor Author

loklokyx commented Dec 2, 2024

Hi @yunho7687, thank you for the feedback and clarification!

I’ve addressed the issue with:

  • remove the unnecessary app directory
  • rename admin folder to manager to avoid conflict with Django's default admin namespace
  • update all relevant file paths and imports to reflect the new directory name
  • integrate the new sub-app in INSTALLED_APPS

I appreciate the reference to the repos and the additional StackOverflow resource. Please let me know if further adjustments are required. Thanks again.

@loklokyx loklokyx requested a review from yunho7687 December 2, 2024 15:39
@loklokyx
feat(SearchSelect): revert api.ts, integrate shadcn select, redesign …
3198621 
…SearchDropdown, and rename component

- revert: restore `api.ts` to its original state
- feat(ui): integrate `@radix-ui/react-select` package to follow ShadCN standards
- refactor(SearchDropdown): redesign component to align with ShadCN select standards
- refactor(SearchDropdown): rename to `SearchSelect` for clarity
@loklokyx
docs: add JSDoc comments for client-side files
25f5ac6 
- docs(button.tsx, search.tsx, select.tsx, table.tsx): add simple JSDoc for shadcn components
- docs(leaderboard-list.tsx, leaderboard.tsx): add JSDoc for page components
- docs(use-fetch-data.ts): add JSDoc for data fetching functions
- docs(leaderboard.ts): add JSDoc for leaderboard type definition
@loklokyx
docs(manager): add pydoc comments for server-side files
ed2b46e 
- docs(serializers.py): add pydoc comments and TODO list for future improvements
- docs(urls.py): add pydoc comments to URL configurations
- docs(views.py): add pydoc comments to views class
@loklokyx
Copy link
Contributor Author

loklokyx commented Dec 4, 2024 •
edited
Loading

Hi Team,

With the cfc standard, I’ve added JSDoc and pydoc comments to the manager module for better clarity and documentation. Please help review the changes and let me know if any adjustments are needed.

Thanks!

Julie-Salazar
Julie-Salazar approved these changes Dec 5, 2024
View reviewed changes
Copy link
Contributor

@Julie-Salazar Julie-Salazar left a comment

Choose a reason for hiding this comment

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

Awesome work lgtm, you do have an empty migrations folder but it is understandable given you have set this up, really appreciate the comment documentation 🙌 I'm good to approve this for now, @yunho7687 might have notes tho

yunho7687
yunho7687 reviewed Dec 6, 2024
View reviewed changes
Copy link
Member

@yunho7687 yunho7687 left a comment

Choose a reason for hiding this comment

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

Legend work!
I’ve decided to hold off on this PR for now until we have a discussion about it in the next session.

client/src/hooks/use-fetch-data.ts
}

/**
* A custom hook that fetches data from an API using the `useQuery` hook from `react-query`.
Copy link
Member

Choose a reason for hiding this comment

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

I think this is an excellent example of how to write documentation, and it's worth us all following as a standard.

@yunho7687 yunho7687 merged commit 4e806b1 into main Dec 7, 2024
5 checks passed
@yunho7687 yunho7687 deleted the issue-7-Admin_Portal_Wireframes_-_Leaderboard_Student_Rego branch December 7, 2024 08:12
@loklokyx loklokyx restored the issue-7-Admin_Portal_Wireframes_-_Leaderboard_Student_Rego branch February 7, 2025 06:39
@loklokyx loklokyx deleted the issue-7-Admin_Portal_Wireframes_-_Leaderboard_Student_Rego branch February 7, 2025 07:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yunho7687 yunho7687 yunho7687 approved these changes

@Julie-Salazar Julie-Salazar Julie-Salazar approved these changes

@AarifLamat AarifLamat Awaiting requested review from AarifLamat

Assignees

@loklokyx loklokyx

Labels
backend Task must have a back end issue enhancement New feature or request frontend Task must have a front end issue
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Admin Portal Wireframes - Leaderboard (Student Rego)
3 participants
@loklokyx @yunho7687 @Julie-Salazar