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

Add star field with GitHub contributors #3149

Merged
merged 1 commit into from
Mar 23, 2022

Conversation

dbelokon
Copy link
Contributor

@dbelokon dbelokon commented Mar 9, 2022

Issue This PR Addresses

#2982 (there are other ideas on the thread that I would like to implement)

Type of Change

  • Bugfix: Change which fixes an issue
  • New Feature: Change which adds functionality
  • Documentation Update: Change which improves documentation
  • UI: Change which improves UI

Description

This PR shows a prototype for a special starfield that will appear in the About page.

Steps to test the PR

  1. Pull my changes (git fetch dbelokon, you have to have my repo as a remote repository)
  2. Checkout the branch (git checkout starfield).
  3. Start the project (pnpm dev). I recommend trying with env.staging as the .env.
  4. Checkout the localhost:8000/about/ page 😄

Checklist

  • Quality: This PR builds and passes our npm test and works locally
  • Tests: This PR includes thorough tests or an explanation of why it does not
  • Screenshots: This PR includes screenshots:
    image
  • Documentation: This PR includes updated/added documentation to user exposed functionality or configuration variables are added/changed or an explanation of why it does not(if applicable)

@gitpod-io
Copy link

gitpod-io bot commented Mar 9, 2022

Copy link
Contributor

@humphd humphd left a comment

Choose a reason for hiding this comment

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

@dbelokon this is really, really creative. I love having a node to "Processing" in Telescope, since Seneca worked for so many years on Processing.js.

I want to figure out where to put this. I'm not convinced that it's appropriate on the landing page of the site. I don't want to remove the dynamic images, which was an important design feature of another Telescope cohort.

Because this is primarily about Telescope's development vs. use, does it belong in the About page or Docs?

@tpmai22 tpmai22 requested a review from sirinoks March 9, 2022 16:29
@tpmai22
Copy link
Contributor

tpmai22 commented Mar 9, 2022

@sirinoks how do you feel about this ? @dbelokon so creative !

@tcvan0707
Copy link
Contributor

It would be great if we have the startfield, but we still need to keep the dynamic images (which we are currently have now). Why don't you try to put the startfield on top of the images?

@cindyorangis
Copy link
Contributor

cindyorangis commented Mar 9, 2022

I followed your instructions and I changed the surpriseNumber because I couldn't find the specialNumber :P

I was able to get this running locally and it looks amazing. I had this running for like 15 minutes and I hit an API rate limit

"message": "API rate limit exceeded for [redacted due to privacy concerns]. (But here's the good news: Authenticated requests get a higher rate limit. Check out the documentation for more details.)",
    "documentation_url": "https://docs.github.com/rest/overview/resources-in-the-rest-api#rate-limiting"

@DukeManh
Copy link
Contributor

DukeManh commented Mar 9, 2022

This is very creative @dbelokon. We need to think about where we can put this, whether the about page or dedicated /contributors/ page or even the status page where we can put more work creative work with p5js there.

@dbelokon
Copy link
Contributor Author

dbelokon commented Mar 9, 2022

@dbelokon this is really, really creative. I love having a node to "Processing" in Telescope, since Seneca worked for so many years on Processing.js.

Thank you!

I want to figure out where to put this. I'm not convinced that it's appropriate on the landing page of the site. I don't want to remove the dynamic images, which was an important design feature of another Telescope cohort.

Because this is primarily about Telescope's development vs. use, does it belong in the About page or Docs?

Thinking about it, it is true that the star field focuses on Telescope project contributors, so moving it somewhere would be better. I would like to move it to the About page, so I don't have to figure out how Docusaurus can bundle p5.js in the website.

@dbelokon
Copy link
Contributor Author

dbelokon commented Mar 9, 2022

I followed your instructions and I changed the surpriseNumber because I couldn't find the specialNumber :P

Whoops~! That's a huge typo 😅 I corrected it to surpriseNumber === 42.

I was able to get this running locally and it looks amazing. I had this running for like 15 minutes and I hit an API rate limit

"message": "API rate limit exceeded for [redacted due to privacy concerns]. (But here's the good news: Authenticated requests get a higher rate limit. Check out the documentation for more details.)",
    "documentation_url": "https://docs.github.com/rest/overview/resources-in-the-rest-api#rate-limiting"

I think the API rate-limiting happens only when you refresh the page several times, requesting the list of contributors so many times. I am not sure why you reached a limit if you only left the page running. Well, with something as complicated as this, it is expected to have bugs 😂

I also edited your comment and removed the previous revision to hide the IP address shown. That's your public IP, and we are in a public repo, so we better hide it 👍

@dbelokon
Copy link
Contributor Author

dbelokon commented Mar 9, 2022

This is very creative @dbelokon. We need to think about where we can put this, whether the about page or dedicated /contributors/ page or even the status page where we can put more work creative work with p5js there.

We can go with @humphd's suggestion to move it to the about page, for sure 👍

@dbelokon
Copy link
Contributor Author

dbelokon commented Mar 9, 2022

It would be great if we have the startfield, but we still need to keep the dynamic images (which we are currently have now). Why don't you try to put the startfield on top of the images?

I think this would be interesting. I didn't think of using the dynamic image as the background for the star field. But when I was reading @humphd reasoning to move this somewhere else, I thought it is better to do that instead of shoving it in the hero banner. Very cool idea, though!

@dbelokon dbelokon self-assigned this Mar 9, 2022
@tpmai22
Copy link
Contributor

tpmai22 commented Mar 9, 2022

@dbelokon you want to push this in 2.8 ?

@JerryHue JerryHue changed the title Add startfield with GitHub contributors Add star field with GitHub contributors Mar 9, 2022
@JerryHue
Copy link
Contributor

JerryHue commented Mar 9, 2022

@dbelokon, this is very cool! I just noticed some strange flickering, where some profile pictures would appear somewhat big and then disappear. Do you know the reason why?

I also updated your original PR message to include the newest changes :)

@cindyorangis
Copy link
Contributor

It looks much better on the About Page. As @JerryHue mentioned, I also see the flickering I can't seem to figure out what's causing it.

Side note: I can't get over how you managed to get something working using a video from 2016 + React + GitHub API like wow. I love visuals and this is just amazing. Gives me https://www.youtube.com/watch?v=WAJ_T2rcKhU @humphd vibes

@humphd
Copy link
Contributor

humphd commented Mar 9, 2022

I wonder if this should go at the top vs. bottom of the About page? It seems wasted "below the fold."

@tpmai22
Copy link
Contributor

tpmai22 commented Mar 11, 2022

@dbelokon how are we heading with this ?

@dbelokon
Copy link
Contributor Author

@dbelokon how are we heading with this ?

@tpmai22, done! I rebased to fix a few conflicts, so it should be done for one final review session :)

@DukeManh
Copy link
Contributor

@dbelokon, I notice some avatars flickering a few seconds in.

@dbelokon dbelokon requested review from joelazwar and RC-Lee March 16, 2022 21:28
@dbelokon
Copy link
Contributor Author

Done! @rclee91 😁

pnpm-lock.yaml Outdated
@@ -125,7 +125,7 @@ importers:
'@babel/preset-env': 7.16.11_@[email protected]
'@babel/preset-react': 7.16.7_@[email protected]
'@babel/preset-typescript': 7.16.7_@[email protected]
'@senecacdot/eslint-config-telescope': link:tools/eslint
'@senecacdot/eslint-config-telescope': 1.0.0
Copy link
Contributor

Choose a reason for hiding this comment

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

These ones here are causing the eslint fails.
It should have been changed in #3220
cc @DukeManh

Copy link
Contributor

@DukeManh DukeManh Mar 16, 2022

Choose a reason for hiding this comment

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

In #3200, I added the no-linking option to pnpm, which won't link any package. But we wanted to link eslint so I linked eslint manually with pnpm link command, pnpm-lock is updated, but when we install a new dependency which this pr does, pnpm sees the no-linking option and unlinks it.

I realized this today and make a fix for it in #3228. We should get it in before Dianna lands this.

RC-Lee
RC-Lee previously approved these changes Mar 17, 2022
Copy link
Contributor

@RC-Lee RC-Lee left a comment

Choose a reason for hiding this comment

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

Approving and ignoring the ESLint failures for now

@RC-Lee RC-Lee dismissed their stale review March 18, 2022 01:09

Some ESLint stuff has updated again, wait for a bit to rebase and pick up on those changes

@RC-Lee
Copy link
Contributor

RC-Lee commented Mar 18, 2022

@dbelokon
Could you rebase and deal with the conflicts once more? Thanks!

@dbelokon
Copy link
Contributor Author

oh oopsie, sorry, @rclee91 , going to do that today

@JerryHue JerryHue self-requested a review March 22, 2022 12:19
@AmasiaNalbandian AmasiaNalbandian requested review from AmasiaNalbandian and removed request for JerryHue March 22, 2022 12:19
@JerryHue JerryHue self-requested a review March 22, 2022 12:20
@TueeNguyen
Copy link
Contributor

I think this one needs a rebase only, there are other issues already filed. @dbelokon

@DukeManh
Copy link
Contributor

@dbelokon, it's safe to delete pnpm-lock and run pnpm install to generate it if you have a problem with conflicts.

@dbelokon
Copy link
Contributor Author

@JerryHue, @AmasiaNalbandian, I have rebased!

Copy link
Contributor

@TueeNguyen TueeNguyen left a comment

Choose a reason for hiding this comment

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

Just tested again, everything works

@JerryHue
Copy link
Contributor

I'll merge when the checks are finished!

@JerryHue
Copy link
Contributor

Checks are done. Merging!

@JerryHue JerryHue merged commit 454030c into Seneca-CDOT:master Mar 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants