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

Contributing.md Enhancement #1683

Merged
merged 21 commits into from
Feb 4, 2025
Merged

Conversation

chrisdel101
Copy link
Contributor

@chrisdel101 chrisdel101 commented Nov 14, 2024

This is a redo of #1671 (which was bungled) and a realization of #1608.

This changes are all made to the CONTRIBUTING.md allowing this file to remain in the repo. This is then copied into the /resources/contributing.md page (at the bottom).

All the text content here is fair game for edits, so if any language is unsuitable, something is incorrect, or you just don't like something it can be changed or removed.
The idea here was just to give a more detailed outline of how to contribute, so if the workflow for that is wrong, it can be corrected.
I left the translation sections in, but with notices of suspension.
Also looks like I cannot run the update-external-docs.yml myself. This needs to be run to ensure that the script is copied over properly, and for the site contributing page to get populated.

Copy link

netlify bot commented Nov 14, 2024

Deploy Preview for expressjscom-preview ready!

Name Link
🔨 Latest commit 00f3710
🔍 Latest deploy log https://app.netlify.com/sites/expressjscom-preview/deploys/67a1821ace0c5d0008dab7ad
😎 Deploy Preview https://deploy-preview-1683--expressjscom-preview.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@chrisdel101 chrisdel101 marked this pull request as ready for review November 14, 2024 22:57
@chrisdel101 chrisdel101 changed the title Contributing Contributing.md Enhancement Nov 15, 2024
css/uz.css Outdated Show resolved Hide resolved
@bjohansebas
Copy link
Member

@chrisdel101 let's try to only modify things related to contributing in this PR; I'll review this PR in detail over the weekend.

@chrisdel101 chrisdel101 force-pushed the contributing branch 3 times, most recently from 4283961 to 44cb937 Compare November 17, 2024 17:20
@chrisdel101
Copy link
Contributor Author

@chrisdel101 let's try to only modify things related to contributing in this PR; I'll review this PR in detail over the weekend.

Done.

Please run the pipeline update-external-docs.yml to check it works.

CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
Copy link
Member

@bjohansebas bjohansebas left a comment

Choose a reason for hiding this comment

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

The work looks great

@bjohansebas bjohansebas requested a review from a team November 17, 2024 17:59
chrisdel101 pushed a commit to chrisdel101/expressjs.com that referenced this pull request Nov 18, 2024
Copy link
Member

@UlisesGascon UlisesGascon left a comment

Choose a reason for hiding this comment

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

Great work @chrisdel101! ❤️

Copy link
Member

@bjohansebas bjohansebas left a comment

Choose a reason for hiding this comment

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

Awesome

@chrisdel101
Copy link
Contributor Author

i know this review might have taken extra work to get it sounding right so my thanks to everyone for helping to review this one.

@bjohansebas
Copy link
Member

Maybe I'll have to wait for @crandmck approval, since it won't let me merge until the required changes review is removed.

Copy link
Member

@crandmck crandmck left a comment

Choose a reason for hiding this comment

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

The content looks good. My comments are mostly copy-edits. While these aren't critical, since this page is specifically about doc contributions, it should be reasonably exemplary.

I made a number of inline comments. Also, all the headings need to be revised to be in sentence case, per the documentation style guide.

CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated
1. **Website Related**:
If you see anything on the site that could use a tune-up, think about how to fix it.

- display or screen sizing problems
Copy link
Member

Choose a reason for hiding this comment

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

Begin bullet-list items with a capital letter.

CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
Chris Del added 2 commits January 19, 2025 12:55
@chrisdel101
Copy link
Contributor Author

chrisdel101 commented Jan 20, 2025

Hi @crandmck. I made all the changes and actually removed quite a bit of other superfluous language. Can you please review again?
@bjohansebas After this is good to go can you please run the action script again? It's been a long while since it was verified with many changes since then. It actually would not run locally so I had to add a semicolon after the final d, and that was not required before, so hoping it still works. I can also remind you when it is time to do this.
sed -E '/^>\[!NOTE\]*/{N;d;}' | \
Also looks like some unwanted files in my git history which I need remove still. Just noticed this.

@bjohansebas
Copy link
Member

The script works fine locally, and I don't see anything unusual. Once this is merged, I will run the script on GitHub.

@chrisdel101 chrisdel101 force-pushed the contributing branch 2 times, most recently from 67871a5 to dcc7c8f Compare January 22, 2025 22:55
@chrisdel101
Copy link
Contributor Author

So I keep getting unwanted changes in my commits. It happens when I merge I think. This has happened again here and it has ruined my commit history. I don't see anyone else complaining about this so not sure why it happens to me so much but it's happened again here.

Anyway, the script isn't working correctly now which I'll need to fix. Might be a result of the git resets I had to do.

The copy is hopefully still okay and can still be reviewed in the meantime.

@bjohansebas
Copy link
Member

I think it was a good idea to enable them to update their branch from the GitHub interface.

@chrisdel101 I believe I can fix your history now since I have the necessary permissions to do so.

@bjohansebas
Copy link
Member

@chrisdel101 I think everything is good now, right?

@chrisdel101
Copy link
Contributor Author

chrisdel101 commented Jan 23, 2025

@chrisdel101 I think everything is good now, right?

Yes it looks okay. Thanks for fixing that, however you did it! I've been pulling into my local branch but this has consistently led to problems. What's the best way to merge?
Can it be merged on github before the PR is accepted? I think I have permissions to do that now.

@bjohansebas
Copy link
Member

What's the best way to merge?

image
You can use this new button on GitHub that I activated, so you can get the latest changes from the upstream main branch.

Can it be merged on github before the PR is accepted?

@chrisdel101 No, you always have to wait until there are no required changes (especially when it's a team member) and I have approval from someone with write permissions (in some cases, it will be necessary for a TC member to approve it as well). If you want, you can contact @crandmck on Slack to get his approval if he wants. Once there are no more required changes and we all agree, you can merge this.

- remove {{content}} as it renders in liquid
@chrisdel101
Copy link
Contributor Author

chrisdel101 commented Feb 1, 2025

Alrighty I fixed the script file and it's working as expected. I tested it on my action and it ran. It didn't like the lines like these (#expressjs-website-contributing) and {#expressjs-website-contributing}, so needed to add logic to handle those.
Hoping we can get this one put away soon :) @crandmck could you check out your requested changes?
Btw you can see the CONTRIBUTING.md here

@bjohansebas bjohansebas requested a review from crandmck February 2, 2025 23:08
Copy link
Member

@crandmck crandmck left a comment

Choose a reason for hiding this comment

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

Thanks for sticking with it and seeing this one through!

@crandmck crandmck requested review from a team as code owners February 4, 2025 02:57
@crandmck crandmck merged commit 3536239 into expressjs:gh-pages Feb 4, 2025
7 checks passed
chrisdel101 added a commit to chrisdel101/expressjs.com that referenced this pull request Feb 16, 2025
Co-authored-by: Chris Del <[email protected]>
Co-authored-by: chris del <[email protected]>
Co-authored-by: Sebastian Beltran <[email protected]>
Co-authored-by: Rand McKinney <[email protected]>
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.

6 participants