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 codes for alumni page #94

Merged
merged 6 commits into from
May 12, 2020
Merged

Refactor codes for alumni page #94

merged 6 commits into from
May 12, 2020

Conversation

yechs
Copy link
Member

@yechs yechs commented May 9, 2020

Major Changes:

  • Add TypeScript type checking for majority of codes
  • Need Help: make the types more granular
  • Separate alumniData from code logic

Minor improvements:

  • Implement a useBaseUrl hook on the URL at the component level, so that the site can now be deployed to another baseUrl without changing the (previously absolute) url of images
  • Fixed a bug for image alt text to overflow
  • Add editorconfig for cross-platform collaboration

yechs added 3 commits May 9, 2020 15:25
- Add TypeScript type checking for majority of codes
- **Need Help**: make the types more granular
- Separate alumniData from code logic
Implement a useBaseUrl hook on the URL at the component level,
so that the site can now be deployed to another `baseUrl` without
changing the (previously absolute) url of images
- Fixed a bug for image alt text to overflow
- Add editorconfig for cross-platform collaboration
@yechs yechs added the enhancement New feature or request label May 9, 2020
@yechs yechs requested review from SamChou19815, madaomax and a team May 9, 2020 16:06
@yechs yechs self-assigned this May 9, 2020
@netlify
Copy link

netlify bot commented May 9, 2020

Deploy preview built successfully 🎉

Built with commit f4bc8fa

https://deploy-preview-94--cocky-bohr-d5033d.netlify.app

Comment on lines 6 to 16
export type Member = {
// should implement a few custom types later
readonly name: string;
readonly image: string;
readonly website: string;
readonly github: string;
readonly linkedin: string;
readonly bio: string;
readonly favoredLink: string;
readonly bio?: string; // should be shorter than 120 characters
readonly image?: string;
readonly website?: string;
readonly github?: string;
readonly linkedin?: string;
readonly email?: string; // should begin with "mailto:"
readonly favoredLink?: "website" | "github" | "linkedin" | "email";
};
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm thinking about making these types more granular

For example:

  • email must begin with mailto: followed by the email address
  • bio should be a string no longer than 120 characters
  • linkedin and github should begin with their domain

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm still going through the TypeScript Manuals for ideas
Let's hope I do find something helpful

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm thinking about making these types more granular

For example:

  • email must begin with mailto: followed by the email address

  • bio should be a string no longer than 120 characters

  • linkedin and github should begin with their domain

This is impossible to achieve in typescript

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm thinking about making these types more granular

For example:

  • email must begin with mailto: followed by the email address

  • bio should be a string no longer than 120 characters

  • linkedin and github should begin with their domain

This is impossible to achieve in typescript

That's sad : (
I was just reading through this issue microsoft/TypeScript#6579 and thought if there exists a workaround for now

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll see if i can find a solution by tomorrow.

Otherwise we'll have to leave it like this. Implementing a format check during run-time would be unnecessary (plus unwise).

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll see if i can find a solution by tomorrow.

Otherwise we'll have to leave it like this. Implementing a format check during run-time would be unnecessary (plus unwise).

In general that will require a dependent type system, and that's usually not what most of the programming language can provide.

Copy link
Member Author

Choose a reason for hiding this comment

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

In general that will require a dependent type system, and that's usually not what most of the programming language can provide.

Thanks for pointing that out! I'm still pretty new to these PL concepts and will definitely look more into things related with type systems.

@SamChou19815
Copy link
Contributor

You can also follow the instructions in facebook/docusaurus#2687 to setup type declaration files to avoid red squiggly lines below @docusaurus/*, etc

@yechs
Copy link
Member Author

yechs commented May 10, 2020

You can also follow the instructions in facebook/docusaurus#2687 to setup type declaration files to avoid red squiggly lines below @docusaurus/*, etc

Wow thanks I was just Googling for that yesterday!

yechs added 3 commits May 10, 2020 07:56
- Remove unneeded type annotations (which can be figured out with type inference, thanks for @SamChou19815 for pointing out)
- Add a few comments to make the Member type clearer for new maintainers
- Remove `null` entries from alumniData
Using AirBnb style with support to React JSX and TypeScript

Yet ESLint still gives error "import/no-unresolved: Unable to resolve path to module '@theme/Layout'."

Also, there are lots of other errors (regarding codestyle) that I will fix later with another PR
@yechs yechs merged commit 8370ad7 into master May 12, 2020
@yechs yechs deleted the sy-refactor-alumni branch May 12, 2020 02:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants