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

refactor(homepage): project card refinement #298

Merged
merged 21 commits into from
Oct 31, 2023

Conversation

ben196888
Copy link
Collaborator

@ben196888 ben196888 commented Oct 19, 2023

Refine project card design

Major

  • remove project card images and replace them with job avatars

minor

  • remove deprecated style and components
  • adjust the layout design on large and medium screen

Impaction

Screenshots

Before

Screenshot 2566-10-31 at 20 04 26 Screenshot 2566-10-31 at 20 04 23 Screenshot 2566-10-31 at 20 04 16

After

Screenshot 2566-10-31 at 22 09 35 Screenshot 2566-10-31 at 20 30 32 Screenshot 2566-10-31 at 20 30 30

Performance

Build and deploy time

item Before After
w/ Cached 00m00s 00m00s
w/o Cached 00m00s 00m00s

@netlify
Copy link

netlify bot commented Oct 19, 2023

Deploy Preview for openstartervillage ready!

Name Link
🔨 Latest commit 027eca5
🔍 Latest deploy log https://app.netlify.com/sites/openstartervillage/deploys/654119a68d9ce60008e39e45
😎 Deploy Preview https://deploy-preview-298--openstartervillage.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
Lighthouse
Lighthouse
1 paths audited
Performance: 93 (🟢 up 11 from production)
Accessibility: 95 (no change from production)
Best Practices: 92 (no change from production)
SEO: 92 (no change from production)
PWA: -
View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to your Netlify site configuration.

@netlify
Copy link

netlify bot commented Oct 19, 2023

Deploy Preview for openstartervillage-canary ready!

Name Link
🔨 Latest commit 027eca5
🔍 Latest deploy log https://app.netlify.com/sites/openstartervillage-canary/deploys/654119a6a9695d0008f1c005
😎 Deploy Preview https://deploy-preview-298--openstartervillage-canary.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
Lighthouse
Lighthouse
1 paths audited
Performance: 88 (🟢 up 1 from production)
Accessibility: 95 (no change from production)
Best Practices: 92 (no change from production)
SEO: 92 (no change from production)
PWA: -
View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to your Netlify site configuration.

@ben196888 ben196888 changed the title Update Card “公務員” refactor(homepage): project card refinement Oct 19, 2023
@ljc1991 ljc1991 marked this pull request as ready for review October 23, 2023 10:01
@ben196888
Copy link
Collaborator Author

Very nice proof of concept idea. I think we should consider these two methods to make this pull request better.

  1. Split Cards component into two or three Cards components. Project cards are styling differently from job and event cards. It's nice to have more Cards component to render different card styling
  2. Move off the colour mapping logic from client side to server side. The colour of background and border should be provided from CMS items. Hardcoded the colour mapping in the server side and render accordingly in the client side makes migrating colour property to CMS items easier.

@ben196888 ben196888 force-pushed the refactor/project-card-refinement branch from 1214340 to c9734f7 Compare October 24, 2023 01:14
@ben196888
Copy link
Collaborator Author

ben196888 commented Oct 29, 2023

The Card component renders ProjectCard and non-ProjectCard differently. The naming of those files are confusing.
I think it would make more sense if we specify a new ProjectCard component and make a DefaultCard component to support the other card types.

Please extract out the ProjectCard JSX to be ProjectCard component and rename nonProjectCard component to be DefaultCard component.

The pseudo code in Card component will be something like follows.

const Card = (props) => {
  if (props.data.type === 'project_card') {
    return <ProjectCard {...props} />
  }
  return <DefaultCard {...props} />
}

@ljc1991
Copy link
Collaborator

ljc1991 commented Oct 29, 2023

The Card component renders ProjectCard and non-ProjectCard differently. The naming of those files are confusing. I think it would make more sense if we specify a new ProjectCard component and make a DefaultCard component to support the other card types.

Please extract out the ProjectCard JSX to be ProjectCard component and rename nonProjectCard component to be DefaultCard component.

The pseudo code in Card component will be something like follows.

const Card = (props) => {
  if (props.data.type === 'project_card') {
    return <ProjectCard {...props} />
  }
  return <DefaultCard {...props} />
}

Yes, Sir! 🫡

@ben196888 ben196888 force-pushed the refactor/project-card-refinement branch from 43e4dd3 to d4d0a9e Compare October 31, 2023 12:59
@ben196888 ben196888 merged commit 7d7392f into main Oct 31, 2023
8 checks passed
@ben196888 ben196888 deleted the refactor/project-card-refinement branch November 2, 2023 15:23
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.

2 participants