Skip to content

Conversation

@hanminh1203
Copy link
Collaborator

@hanminh1203 hanminh1203 commented Dec 20, 2025

Change Summary

Improved responsive design and refactored artwork pages to follow Next.js best practices. Centralized mock data generation into a reusable hook and added proper error handling for empty database states.

Changes Made

  • Refactored ButtonGallery component to use Next.js Link component properly
  • Added placeholder SVG icon for artworks without images
  • Centralized mock data generation in useArtworkData.ts hook
  • Added error handling in getServerSideProps to return mock data when API fails or DB is empty
  • Improved responsive layouts with proper spacing and mobile-first approach
  • Fixed TypeScript type issues with Next.js route parameters and PageResult types

Change Form

  • The pull request title has an issue number (Individual art pages #8)
  • The change works by "Smoke testing" or quick testing
  • The change has tests - NA: Mock data hook doesn't require unit tests at this stage
  • The change has documentation - NA: Changes are self-explanatory code refactoring

Other Information

  • Add module django_filters.rest_framework - need confirmation

Testing Notes:

  • Tested with empty database - placeholder icons display correctly
  • Tested navigation between gallery and individual artwork pages
  • Verified responsive layout on mobile and desktop viewports

Related issue

Copy link
Collaborator

@DDuu123321 DDuu123321 left a comment

Choose a reason for hiding this comment

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

Thanks for the thorough review, James. I've made the following changes:

  1. Removed django-filter dependency:
    Deleted the DjangoFilterBackend import and related configuration from views.py
    Removed django-filter==24.3 from requirements.txt

  2. Refactored database models:
    Removed social media fields (discord_url, instagram_url) from the Member model
    Deleted the erroneous Migration 0007 (which attempted to add social media fields to ArtContributor)
    Kept ArtContributor as a pure association table containing only art, member, role
    Updated related serializers to remove social media field references

  3. Simplified URL configuration:
    Simplified the router configuration in urls.py as requested

  4. Other improvements:
    Replaced unique_together with UniqueConstraint
    Added default=True to Art.active

All changes have been pushed, and the code now fully conforms to the original schema design. Thank you for your careful review. During multiple rounds of modifications, I lost sight of the original design requirements and added a lot of code based on my own assumptions without consulting first. I apologize for that.

@Karl-Sue
Copy link
Collaborator

The latest version of the code resolved two tickets:

  • Removed pagination
  • Changed from ViewSet to RetrieveAPIView

@laurenpudz laurenpudz self-requested a review January 21, 2026 09:39
Copy link
Contributor

@laurenpudz laurenpudz left a comment

Choose a reason for hiding this comment

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

hello!! could you please resolve the conflicts between this branch and the main branch, thanks!

@laurenpudz
Copy link
Contributor

Hey! Seems like some conflicts have cropped up since the new events page was merged into main. Please could you resolve these too!

Copy link
Collaborator

Choose a reason for hiding this comment

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

The changes to the poetry lock should be reverted since the python packages used haven't changed.

fontFamily: {
sans: ["var(--font-sans)", ...fontFamily.sans],
jersey10: ["var(--font-jersey10)", ...fontFamily.sans],
jersey10: ["Jersey 10", ...fontFamily.sans],
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why was this font's definition changed?

sans: ["var(--font-sans)", ...fontFamily.sans],
jersey10: ["var(--font-jersey10)", ...fontFamily.sans],
jersey10: ["Jersey 10", ...fontFamily.sans],
dmSans: ["DM Sans", ...fontFamily.sans],
Copy link
Collaborator

Choose a reason for hiding this comment

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

This font may need to be included in the _app.tsx like the others to make sure the font will be available for all browsers.

sans: ["var(--font-sans)", ...fontFamily.sans],
jersey10: ["var(--font-jersey10)", ...fontFamily.sans],
jersey10: ["Jersey 10", ...fontFamily.sans],
dmSans: ["DM Sans", ...fontFamily.sans],
Copy link
Contributor

Choose a reason for hiding this comment

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

We actually shouldn't need this font at all as we already have another sans font in the project. You can remove this and just use font-sans instead. Having two very similar but slightly different fonts used inconsistently will not be great aesthetics-wise

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.

Individual art pages

6 participants