-
Notifications
You must be signed in to change notification settings - Fork 6
added contributing page #97
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey Matt, great work on getting this page up and running! I have some broad areas of feedback:
- copy (which I think is really important) - mostly threaded, though I didn't really give feedback on the contribution workflow section. I think conciseness and simple language is super important for this page.
- the "good first issue" feature - below
Good First Issue
I really like the Good First Issue idea! Some suggestions:
- as I mentioned below, I think inlining the entire list is pretty disruptive; you may want to figure out a different way to deal with that
- to me, repositories-first is a bit confusing for users. my suggestion is to either:
- (easy) preload the link to have the "good first issue" label pre-applied, like this: https://github.com/uclaacm/opensource/issues?q=is%3Aissue+is%3Aopen+label%3A%22good+first+issue%22
- (harder) link the issues themselves. I'm personally a fan of how https://goodfirstissue.dev/ does it, but there's a bit of creative freedom you have in how you want to do this.
I have some minor styling feedback, but that's probably not necessary for this PR. Great work as always!
pages/contribute.tsx
Outdated
how can i contribute? | ||
</h2> | ||
<p> | ||
We'll quickly go over how we can use git/github to work on some open source projects, and if |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where are you going over how to use git/GitHub?
(also, you can probably split up the run-on sentence here)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We quickly go over cloning, forking, branching, etc. But, it could probably be hashed out better (maybe in a content PR?)
pages/contribute.tsx
Outdated
<h2 id='good-first-issues'> | ||
acm's projects w/ good first issues | ||
</h2> | ||
<div className='card'> | ||
<div className='card-body'> | ||
{displayedgfiProjects} | ||
</div> | ||
</div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a pretty large content box; I would suggest either making it collapsible, only showing the first 3, or linking to some other page. If I saw this, I wouldn't know to scroll down and read the other FAQs!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think putting it at the end of the page and linking to it or making it its separate page would work, would love to hear your thoughts!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a huge fan of end of page now looking at it. I think separate page makes the most sense, and add it to the navbar/the front four cards (since some of those links are meaningless right now).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, we'll definitely have to figure out how to sort out the navbar if we'll have the ucla-opensource
page up there as well, it might get a bit crowded. I'll probably add a link to it on the front-page for now.
</div> | ||
</div> | ||
|
||
<h2>contribution workflow</h2> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this section is good, but you can move it to a new page if you'd like; I would say it's different in style from the other things on this page (and it's getting long). Perhaps "making your first pull request".
Also, most of your examples use the CLI. You may want to explain:
- how to download git if you don't have it already (ex, most Windows systems)
- what a CLI is (particularly important for Windows users who may want WSL or will have to learn powershell)
- if they should use something like GitHub Desktop (this is opinionated)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I think the fixing up of this and deciding how we want to structure it can be its own issue and PR as well, but up to you!
restructured the gfi projects, and addressed some content changes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not going to comment too much on the copy / markup if you want to improve it later, but I would definitely suggest giving it a serious pass (being consistent with capitalization and using code
blocks, being more concise, breaking things up into reasonable chunks, etc.). This is more of a design and communications task and less of a developer one, but probably more important than the actual functionality of this page. I think you should really invest some resources into this if you want to get this ready for fall.
I think I still agree with my previous feedback on not writing to a file as a part of this project; I don't think it's the appropriate use-case of Next. I wonder if you should investigate deploying on vercel, using an in-memory cache, or fixing whatever issue you had with SSR and revalidate timers (are you actually deploying with SSR? make sure!).
</h1> | ||
<div className='row spaced-row'> | ||
<h1>get started </h1> | ||
<a href='#good-first-issues'><button>good first issues</button></a> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like wrong markup?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean with the space in the h1 or the button being in the same row as the h1? At first I had the button to the GFI projects on the same line as the header but I'll change that to point to a new page now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah no, in this case I mean having a button inside the anchor. I know it's tied to CSS but it's an accessibillity no-no.
(I know it's a little hypocritical since I banged out the first iteration of this code)
Interesting, I'll see if we can do this stuff with Netlify but the main problem that I see happening is that currently as it is, one person can get the data but if multiple people go to the website then we might overload the system as is :(( I'll look to see if isr is actually being used haha, since it seems as though it's not working as intended. I might ask for some help in the future but i'll lyk! |
I'll def get some opensource enthusiasts to flesh this out and represent opensource in the best light! I'll loop Jason Cheng @jc65536 into this to see if he wants to talk about opensource on this page! |
I really think we're using Netlify wrong; our use-case is so small compared to many other companies, projects, etc. that use it - this is such a common business-case problem (this is one of the driving examples of SSR/ISR/etc.) so I think there's some more research we should do. Good luck on figuring it out, I think this can really pay dividends for the rest of ACM though! |
Oop I just checked our netlify page for opensource and it looks like we're currently on v3.9.2 for the netlify nextjs plugin but ISR support only started on v4.0 and up! Updating it rn |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job, looks perfect!
Made the contributing page and included a couple things:
good-first-issues
from octokitAlso adjusted the project request functions and turned smaller tasks into functions to reduce code smells.