Skip to content

Add a contributing guide for the Next.js site - #244

Open
Dushyant-web wants to merge 3 commits into
OpenPrinting:masterfrom
Dushyant-web:add-contributing-guide
Open

Dushyant-web wants to merge 3 commits into
OpenPrinting:masterfrom
Dushyant-web:add-contributing-guide

Conversation

@Dushyant-web

Copy link
Copy Markdown
Contributor

Thank you @tillkamppeter for merging the Contribute page so quickly.

This PR adds a new CONTRIBUTING.md for the current Next.js site. The old one was removed in the Next.js migration and #185 still has the Jekyll steps, so as discussed in #202 I started fresh and used #185 only as reference.

What's in it:

  • What this repository is for, and where to go for printing problems and software bugs instead
  • Where things live (contents/, data/, public/, app/ and so on)
  • How to write a news post, with the frontmatter fields
  • Setting up locally with Yarn and Corepack, and what yarn dev does not generate
  • Checks before a pull request (yarn lint, yarn verify:urls) and a few repository rules: no trailing slashes in internal links (Broken links all over the site (due to "/" at the end not accepted) #207), use siteConfig and getAssetPath, and don't commit generated files
  • Working with issues, making a pull request, and a short style guide (US spelling, Markdown, sentence case headings)

I tested the setup steps myself. On macOS, SKIP_PPD_GEN=true yarn generate finishes without errors, so the guide mentions it, since the PPD step needs Linux tools. I could not run a full yarn build on macOS.

Only CONTRIBUTING.md is added, nothing else is changed.

I'm still learning, so please tell me if anything should be corrected or explained better.

cc @izmalk @prisorue @n0y0nD

@tillkamppeter

Copy link
Copy Markdown
Member

@rudra-iitm Could you check whether the instructions are correct? Thanks.

@tillkamppeter

Copy link
Copy Markdown
Member

For me it looks all OK.

@rudra-iitm rudra-iitm left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

After these changes, it's LGTM

Comment thread CONTRIBUTING.md

You need [Node.js](https://nodejs.org/) 20 (the version CI uses) and Yarn. The Yarn version is pinned in `package.json`, and Corepack picks it up for you:

```bash

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Should we also add steps to install Node.js and the Yarn package manager??

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Would be a good idea. There could be beginners in web development.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

There is now a short paragraph on installing Node.js (from nodejs.org), how Yarn comes with it through Corepack, and how to check the versions.

Comment thread CONTRIBUTING.md Outdated
- Don't hardcode the site URL or the repository name. Use `siteConfig` from `config/site.config.ts`.
- In code, pass image paths through `getAssetPath()` or `getImageSrc()` from `lib/utils.ts`.
- Don't commit generated files. `public/search/`, `public/foomatic-db/`, `public/query/`, `public/ppds/` and `public/feed.xml` are rebuilt on every build and are in `.gitignore`.
- If `yarn install` changed `yarn.lock`, undo that with `git checkout -- yarn.lock` before you commit.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This is not always true, so we should update this instruction accordingly.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@rudra-iitm Please give us the correct instructions here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks. For now I made it conditional: undo the yarn.lock change only if you didn't change any dependencies on purpose, and commit it only when the pull request really adds or updates a dependency. @rudra-iitm please tell me if there is a better rule and I'll update it.

Comment thread CONTRIBUTING.md

`yarn dev` does not build the printer database or the search index. Run `yarn generate` once if you need the printer and driver pages or the site search locally. The PPD files it creates need Foomatic tools that only exist on Linux, so on macOS run `SKIP_PPD_GEN=true yarn generate` instead. Everything else is still generated. For a full `yarn build` outside Linux, use the dev container in `.devcontainer/` or let CI build your pull request.

## Before you open a pull request

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Should we also add 'yarn build' as necessary? Making sure foomatic db builds and everything works before raising the cl? I know CI checks it, but we should be extra safe before reaching CI.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes, we should actually give the contributor the full instructions so that they can do all the testing before committing to reduce guess work commits.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I added the full instructions: the apt packages for Debian and Ubuntu (same list as CI), and the dev container steps for macOS and Windows, so all three checks (lint, build, verify:urls) can be run before committing.

Comment thread CONTRIBUTING.md
1. Fork the repository and create a branch from the latest `master`.
2. Keep each pull request to one topic.
3. Explain what you changed and why, and link the issue (for example `Fixes #202`).
4. Add screenshots if the change is visible.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Should we mention "media" here instead of "screenshot"?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think, "screenshots" fits well here, as we are working on the web site, not on printing itself (where one would also have input files and output pages.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I agree, screenshots fit better here. I kept it as it is.

@Dushyant-web

Copy link
Copy Markdown
Contributor Author

@rudra-iitm I've addressed all your review comments in the latest commits. Could you please take another look when you have time? For the yarn.lock point I used a conditional rule for now, and I'm happy to change it if you have a better one. Thanks!

This branch has not been deployed

No deployments
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.

3 participants