use octokit + public repository API to generate projects data #57

matthewcn56 · 2021-12-22T08:28:24Z

Instead of using a JSON object or markdown file, this uses octokit to grab all of @uclaacm's repositories to generate the list of ProjectCards.

However, a few modifications were made to the Project interface to make it work with this setup:

tech was changed to topics to use the repository's associated topics instead
Language uses the language provided by the repository as opposed to the Language enum
link can be possibly undefined if the repository doesn't have an associated homepage

There are also a couple approaches we can take to associate an image to each repository, since currently we are unable to associate a unique image to each repository with our octokit request:

First, we can use the social media preview image associated with the repository (which would enforce good practice of adding that in for repository maintainers.)
- In order to use the social media preview image associated with the repository, we would have to use GitHub's graph ql api.
We can also add in an image at the root directory of each repository, and access it through https://github.com/uclaacm/REPO_NAME/blob/main/image.jpg.
Using an iframe instead of an img of the homepage of each project to view how it looks
We can take the approach that Bryan outlined of adding in a config yml file as detailed in feat: turn data/projects into a markdown file #26 .

mattxwang · 2021-12-23T08:18:57Z

Hey @matthewcn56, it doesn't look like this passes lint/build - could you resolve that first?

matthewcn56 · 2021-12-23T08:54:33Z

These errors are in files that I did not change, but I still addressed them.
However, I had to add in a couple es-lint disables since the payload property of the OctoKit response is in fact any, and the lint config disables explicit any. Also, for the customRender function, the return type used something from node_modules so I also had to es-lint disable that.

I think most of the linting errors come from the template files that are artifacts from create-next-app though?

matthewcn56 · 2021-12-23T08:56:47Z

Looks like the test is still failing, do you want to run the test with the projects prop containing junk data or an empty array?

mattxwang · 2021-12-23T20:54:40Z

These errors are in files that I did not change, but I still addressed them.

Hm, I'm not entirely sure what you mean here - your change broke one of the tests, right? The test works properly on main.

Looks like the test is still failing, do you want to run the test with the projects prop containing junk data or an empty array?

I would say neither; instead, you should run the test with an actual API response from GitHub/whatever octokit gives you/whatever input makes sense - this would be a "fixture". Gives you the best test coverage since you're testing it on real data!

matthewcn56 · 2021-12-23T21:56:53Z

Ah sorry, I mean to say that the linter found issues that were pre-existing, my changes did in fact break one of the tests! Will fix rn

matthewcn56 · 2021-12-23T22:15:08Z

@mattxwang I addressed the jest failures, but had to also update the snapshot to do so. Would testing jest snapshots fail in the future though if we randomize the featured project?

matthewcn56 · 2021-12-23T22:45:19Z

also tried using iframe instead of img if the repository contained a link since on the deployment right now, it looks like primarily iframes, except for a couple cases.

If we're able to get committees to add the name of their committee to the topics list of their repository, then we can change the default image from being the acm logo to the committee logo.

mattxwang · 2021-12-23T23:00:15Z

Would testing jest snapshots fail in the future though if we randomize the featured project?

Nope! What we'd want to do is allow an optional prop that overrides the random project, pass that in as a fixture, then test the snapshot against that. Guarantees consistency, etc.

also tried using iframe instead of img if the repository contained a link since on the deployment right now, it looks like primarily iframes, except for a couple cases.

Hm, I would avoid the iframe if you could? I don't think it adds too much to the design, and is a pretty bad network request load. I liked the previous idea of using the repository's social media thing; you could also use the committee logo as a stopgap, that's not a bad idea too!

If we're able to get committees to add the name of their committee to the topics list of their repository, then we can change the default image from being the acm logo to the committee logo.

Do you want to try this on a couple of repos? Interested to see what it'd look like. I think I tagged a few of the Teach LA ones while I was president.

matthewcn56 · 2021-12-24T00:25:38Z

Sounds good, I removed the iframe in favor of the committee logos instead!

If we can get everyone to add the committee topic as outlined in our ACMCommitteeTopics enum or changing it to whatever format everyone can agree on, then it'll work properly! (it worked with teach-la's recent repositories which contained the teach-la topic tag)

Here's an example with a couple repositories with the tag and without pulled from octokit:

matthewcn56 · 2021-12-24T00:55:52Z

Changed the color badge to use the color as specified by Github's Color list for languages and Github's naming convention for languages as opposed to the custom Language enum built originally.

Example of language colors:

matthewcn56 · 2021-12-24T01:07:13Z

I find that whenever I make a change that changes how the page looks, i have to run yarn jest --updateSnapshot in order to pass the PR checks. Is this intended, and if so should we add into the README that developers should run the above command if their changes make the page look different?

mattxwang · 2021-12-24T07:23:42Z

Looks like things are coming together! I can give a formal review tomorrow, but wanted to note that the spacing on the projects page is quite wonky for me.

I find that whenever I make a change that changes how the page looks, i have to run yarn jest --updateSnapshot in order to pass the PR checks. Is this intended, and if so should we add into the README that developers should run the above command if their changes make the page look different?

Yes, if you change the layout of the homepage, we need to regenerate the snapshot. Adding that and/or adjusting the test is a great idea!

I think in this case, it may make more sense to snapshot only certain components or behaviours, but open to feedback/thoughts.

mattxwang

Great first stab @matthewcn56! I think this is a really exciting feature, hopefully this pans out.

I left some technical comments on each code snippet. Some other broader things I want to talk about:

there are a few layout quirks - the images are squashed slightly, the spacing is a bit off, etc.
I think if the repository has no topics, we shouldn't render the dot; it looks a bit strange as-is
you need to add an edge-case for no language; see the centralized-intern-training repository, for example. markdown is probably a reasonable language to put here.
can we add a mechanism to opt things out, and/or filter out archived repositories? perhaps this is something we add as a later feature
I noticed you added a paginate plugin - are you using it? that might not be a bad idea!
if you've imported the octokit types, you should be able to refine the type for the payload down - see fix: define and/or use an existing type for GitHubEventAction #34

Some screenshot-based issues:

long titles don't wrap properly:

Great work so far again! Excited to see what you do with the site 😊

mattxwang · 2021-12-25T01:12:49Z

data/githubColors.json

@@ -0,0 +1,2142 @@
+{


Where did you get this file?

If you got this file from someone, we should definitely credit them

Could we somehow make this a package or dependency instead? While I'm wary of NPM, I feel like this kind of thing shouldn't be committed to our repository/rely on us to update.

The file is from someone's github-colors repository but it's not an npm package. While it doesn't have an NPM package, it is automatically updated. Instead of putting in the json file ourselves, we can also run a get request on the raw json file from the repository at build time instead with the raw content link.

I'm going to grab from the raw content link but I'll use the .json as a fallback.

Agh sorry, I meant to respond to this in my earlier review run - I think we should:

definitely credit them somewhere, probably in the code and the README

consider a more sustainable way to do this than the raw content link or including it in our repo; a submodule comes to mind, though I'm not sure how that would factor in to our build step.

components/ELink.tsx

mattxwang · 2021-12-25T01:15:44Z

.prettierrc

+{
+  "singleQuote": true,
+  "trailingComma": "all"
+}


Not sure if I'm missing something, but prettier is not in this repo / in this branch. Did you mean to add prettier?

If so, can we try to keep the formatting consistent with what already exists? i.e., minimizing the number of changes introduced in this PR?

This overrides the default prettier configs so that developers can use it and it can still stay consistent with what already exists. While it's not necessary, I think it can help remove some of the formatting conflicts that arose in other PRs.

Gotcha; to separate concerns, could we move this to another PR where we also add prettier? I think we removed a while back in another PR (#3 and #4), but it's probably a good idea to reintroduce.

pages/index.tsx

test/pages/index.test.tsx

mattxwang · 2021-12-25T01:19:03Z

test/testUtils.ts

+/* eslint-disable  @typescript-eslint/explicit-module-boundary-types */
+const customRender = (ui : JSX.Element, options = {}) =>
  render(ui, { wrapper: Providers, ...options });
+/* eslint-enable  @typescript-eslint/explicit-module-boundary-types */


Just curious, why does this need to be here? I think some of this was boilerplate code.

Yeah, the linter was yelling at the boilerplate code for not defining the type of the customRender function.

If I run yarn lint in main, the linter fails in the current deployment because of this (and some extra things that were addressed in this PR as well).

Oh I meant more - this seems wrong for boilerplate code, are we using it improperly? Something we can resolve outside of this PR!

mattxwang · 2021-12-25T01:20:05Z

util/projectRequest.ts

+      } as Project),
+  ) as Project[];


Are these as Project typecasts necessary? Are we not able to do structural typing/inference?

(I'm not sure here, too used to using Flow at this point)

If we define the return value of the function in the function declaration then these typecasts aren't necessary. I wasn't used to the linter requiring the function return types at the top of the function declaration so I wrote in the typecasts at first, but after running through the linter and adding in the function return types, these typecasts are redundant.

Without the function declaration at the top or typecasts, then VS.Code's Intellisense wouldn't give the return values their types when we hover over the function, and it would not use our user defined type of the object returned.

Right, that's what I imagined. A couple of suggestions:

you can annotate anonymous functions in Typescript - see More on Functions

this also goes hand-in-hand with a way we can break down the function into constituent parts. While this isn't "mandatory" / always the right advice, we could instead do something like:

function getProjectFromRepo(repo: RepoType): Project { return { name: repo.name, description: repo.description, link: repo.homepage || null, repo: repo.html_url, lang: repo.language, topics: repo.topics, image: getImageFromTopics(repo.topics).image, alt: getImageFromTopics(repo.topics).alt, }; }

and then pass the above function into the map you apply on the sort. While this is a bit more verbose, it can also make the code easier to read/understand, and you can test these functions in isolation. EOD though, up to you!

util/projectRequest.ts

mattxwang · 2021-12-25T01:20:57Z

util/projectRequest.ts

+        alt: 'ACM W Logo',
+      } as ImageInfo;
+    default:
+      return false;


Why not have this return the ACM logo, and them simplify the call in 83 to basically just call into this function? Also has the bonus that the function now always returns ImageInfo.

In getImageFromTopics, some topics can be added that aren't an ACM Committee like learning-labs, education, react, etc. I made the default in this case false so that we can iterate through the list of topics until we reach the name of a committee. If we overrode topicToImg to return the ACM logo as a default, then we would not properly get the logo of the repository's associated committee if the committee name was not the first topic of the repository.

Ah, I see - I think I misunderstood the purpose of this function.

A follow-up question; let's say we add more images in the future that aren't committee-based (maybe, for example, we have a special logo for learning labs). You wouldn't be able to add that to this switch-case, right? Since then the order of the topics would matter (and you can't early-exit out of getImagesFromTopics once you get the first valid topic). Is this something you think we'll run into?

That's a good point! I think that in its current implementation, we can use the getImagesFromTopics function as a way to get a default image for each repository, and it would take the order of least precedence from the other ways we can select images/logos for everything.

I don't think I understand non-image-based logos, since can't all logos be .png files as images?

In the future when we implement og:images and social media previews if possible for repos, we can prioritize special images first and use getImagesFromTopics as a last-resort if the other ways to get images fail.

Maybe we should rename it to getCommitteeImage instead to better outline its use case?

Sorry, image-based is a typo - meant committee-based. Fixed!

Maybe we should rename it to getCommitteeImage instead to better outline its use case?

I trust your judgement as long as you've thought about future-proofing for other use-cases!

mattxwang · 2021-12-25T01:22:23Z

util/types.ts

+  name: string;
+  description: string;
+  repo: string;
+  link: string | null;


I would consider - do you want to make this type nullable, instead of explicitly a union with null?

Making this type nullable would require extra steps in our mapping function of the getProjects function then. Should we add in extra steps of our mapping function to make the link field nullable?

Ah sorry, I miswrote - meant to say optional ("nullable" is not a special type in TS). It is a more generic type than string | null, so I think there's an argument that swings either way. If you think this is the better approach, feel free to keep it as-is.

On second thought, I think nullable does make more sense.

matthewcn56 · 2021-12-27T21:33:45Z

can we add a mechanism to opt things out, and/or filter out archived repositories? perhaps this is something we add as a later feature

Great idea! For now I'll change our getProjects function to automaticallly filter out archived repositories and we can add in a feature to toggle it on/off client-side in a later PR.

I noticed you added a paginate plugin - are you using it? that might not be a bad idea!

The paginate plugin is used to get the list of repositories within our organization, and is necessary if the # of repos > 100.

- Changed behavior of ELink to always generate link if exists. - Changed GitHub Event to use the proper types from octokit. - Addressed no topics or default language for ProjectCard. - Added fixtures for repositoryResponse. - Changed GithubColor behavior to attempt to grab the most recent from the repository.

mattxwang · 2021-12-27T23:04:52Z

The paginate plugin is used to get the list of repositories within our organization, and is necessary if the # of repos > 100.

Yup, I meant more on the client-side; I don't see a way to see everything past the first page (and we have >100 public repos).

matthewcn56 · 2021-12-27T23:07:15Z

Ah, the paginate plugin returns an array of >100, so the client sees the list of all 100+ repos. Maybe in the future we can make multiple pages instead of all repos all on the same page.

mattxwang · 2021-12-28T02:48:42Z

util/types.ts

+  link?: string;
+  image: string;
+  alt: string;
+  lang: string;
  // TODO(mattxwang): if tech is an enum, does that make it easier to search/sort?


You should be able to remove this TODO now btw!

mattxwang · 2022-01-04T23:39:40Z

If Nick's handling the payload type in his PR, should I avoid touching the payload typing and leave it as any for now, or should I merge in his changes first?

I have an inkling that his changes should be forwards compatible, but for now you can chug along with whatever changes you were going to make; we can merge whoever is done first (I think Nick) and have the other person resolve, if you don't mind?

reginawang3495 · 2022-01-07T04:01:47Z

@matthewcn56 We've merged in Nick's changes!! Note the types aren't fully right if that matters (I haven't really read this PR so I don't know) -- see #64

matthewcn56 · 2022-01-08T17:51:31Z

@matthewcn56 We've merged in Nick's changes!! Note the types aren't fully right if that matters (I haven't really read this PR so I don't know) -- see #64

Sounds good! Will address soon then

mattxwang · 2022-01-24T22:19:51Z

FYI this merge conflict is looking pretty nasty - let me know if you need any help @matthewcn56

Punts the GitHub Action typing since #57 will likely resolve it, and resolves/punts boilerplate code errors. Changes getStaticProps typing via [this SO post's instructions](https://stackoverflow.com/questions/65078245/how-to-make-next-js-getstaticprops-work-with-typescript). To resolve testing wrapper props, looks at [this SO Post](https://stackoverflow.com/questions/68460887/whats-the-proper-typing-for-a-react-testing-library-wrapper-in-typescript) - basically, using type inference. Adds optional chaining for tests.

matthewcn56 · 2022-02-19T01:45:40Z

Hmm, it looks like the jest snapshot testing stopped working due to test-file-stub issues, might be related to this issue? vercel/next.js#33976

mattxwang · 2022-03-02T09:10:42Z

Okay, to be completely frank I think there's something broken with the way that we're snapshot testing. With infinite time, I would resolve this, but:

We're in a student organization that's just undergoing a transition
Nobody else maintains this code
We don't even use snapshot tests well

So, with my executive decision, let's remove the snapshot and call it a day 😊

I also fixed a merge conflict and a lint error.

@matthewcn56 if you can address the layout and image problems (at least to put it in a usable/clean state), then I'm willing to merge!

matthewcn56 · 2022-03-02T19:15:41Z

@mattxwang I fixed the layout issues with regards to all cards not being the same size, and now I think our final issue is how can we align each section of the card-body, like the titles, the languages, etc. across different cards as they are all within different flex-containers. The current deployment that's live now also has this issue so maybe this CSS issue can be resolved later! And also, I am puzzled that the footer seems not to be placed properly and am unable to see why this is the case.

mattxwang

Feel free to tackle the metadata alignment later!

I think the footer problem you're running into is the same as this overall spacing problem (I'm on macOS, Chrome):

This vertical spacing should probably only be like 1rem or 2rem at most.

Something about the grid-gap or otherwise from he cards is not working as intended.

If you search for the footer, it's half-way through the page; I think the grid-gap is placing the footer improperly, it's where the end of the page would be without the spacing problem.

matthewcn56 · 2022-03-02T19:50:29Z

yup, hard-setting row-gap fixed the issue!

mattxwang

This is definitely a great starting point for a huge overhaul of the site. I think it's good to merge.

This lets us create a slew of follow up tickets:

a compact mode
biased random featured projects
search
properly using images for projects

and more! Great work @matthewcn56, you should be really proud of this feature!

refactored projects to use github api instead

b42e8d1

matthewcn56 requested a review from mattxwang December 22, 2021 08:28

fixed autoformatting and removed extraneous updated_at

3501bcb

fixed linting runs

58ce766

addressed jest issues

5bbbd56

added iframe for img

fe675b6

removed iframe in favor of committee logos

5979511

added color badges using github data

9205058

jest snapshot updated

05f9f7d

octokit pagination for all repos

6fe0be6

mattxwang requested changes Dec 25, 2021

View reviewed changes

matthewcn56 added 2 commits December 27, 2021 14:26

updated jest snapshot

e4a2068

matthewcn56 requested a review from mattxwang December 27, 2021 22:36

mattxwang reviewed Dec 28, 2021

View reviewed changes

This was referenced Jan 7, 2022

feat: add Passworks, Cipher Salad, Playnet to projects page #32

Closed

add (composable) filters for projects #75

Open

mattxwang mentioned this pull request Jan 14, 2022

Flesh out the projects page #12

Closed

19 tasks

mattxwang mentioned this pull request Feb 3, 2022

turns on strict mode for TSLint, resolves/punts all errors #87

Merged

addressed merge conflicts and properly types github events with octokit

542a925

matthewcn56 and others added 4 commits February 25, 2022 00:21

attempted to fix jest issues

a4e7453

Merge branch 'main' into api-for-projects

637cc09

fixes lint error

deddd8b

removes snapshot test for homepage

c9824b6

fixed layout and image

33b8781

matthewcn56 requested a review from mattxwang March 2, 2022 19:15

removed extraneous div

b415339

mattxwang requested changes Mar 2, 2022

View reviewed changes

mattxwang changed the title ~~refactored projects to use octokit repository get request instead~~ use octokit + public repository list to generate projects list Mar 2, 2022

mattxwang changed the title ~~use octokit + public repository list to generate projects list~~ use octokit + public repository API to generate projects list Mar 2, 2022

mattxwang changed the title ~~use octokit + public repository API to generate projects list~~ use octokit + public repository API to generate projects data Mar 2, 2022

fixed row gap

f97abfa

matthewcn56 requested a review from mattxwang March 2, 2022 19:50

mattxwang approved these changes Mar 3, 2022

View reviewed changes

mattxwang merged commit debeb30 into main Mar 3, 2022

mattxwang deleted the api-for-projects branch March 3, 2022 07:47

use octokit + public repository API to generate projects data #57

use octokit + public repository API to generate projects data #57

Conversation

matthewcn56 commented Dec 22, 2021 • edited Loading

mattxwang commented Dec 23, 2021

matthewcn56 commented Dec 23, 2021

matthewcn56 commented Dec 23, 2021

mattxwang commented Dec 23, 2021

matthewcn56 commented Dec 23, 2021

matthewcn56 commented Dec 23, 2021 • edited Loading

matthewcn56 commented Dec 23, 2021 • edited Loading

mattxwang commented Dec 23, 2021

matthewcn56 commented Dec 24, 2021 • edited Loading

matthewcn56 commented Dec 24, 2021 • edited Loading

matthewcn56 commented Dec 24, 2021

mattxwang commented Dec 24, 2021

mattxwang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattxwang Dec 27, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattxwang Dec 27, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthewcn56 commented Dec 27, 2021

mattxwang commented Dec 27, 2021

matthewcn56 commented Dec 27, 2021

Choose a reason for hiding this comment

mattxwang commented Jan 4, 2022

reginawang3495 commented Jan 7, 2022

matthewcn56 commented Jan 8, 2022

mattxwang commented Jan 24, 2022

matthewcn56 commented Feb 19, 2022

mattxwang commented Mar 2, 2022

matthewcn56 commented Mar 2, 2022

mattxwang left a comment • edited Loading

Choose a reason for hiding this comment

matthewcn56 commented Mar 2, 2022

mattxwang left a comment

Choose a reason for hiding this comment

matthewcn56 commented Dec 22, 2021 •

edited

Loading

matthewcn56 commented Dec 23, 2021 •

edited

Loading

matthewcn56 commented Dec 23, 2021 •

edited

Loading

matthewcn56 commented Dec 24, 2021 •

edited

Loading

matthewcn56 commented Dec 24, 2021 •

edited

Loading

mattxwang Dec 27, 2021 •

edited

Loading

mattxwang Dec 27, 2021 •

edited

Loading

mattxwang left a comment •

edited

Loading