Skip to content

docs: clarify yarn install purpose in README #4629

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open

docs: clarify yarn install purpose in README #4629

wants to merge 13 commits into from

Conversation

@Subham-KRLX
Copy link
Contributor

@Subham-KRLX Subham-KRLX commented Jun 13, 2025 •
edited
Loading

Closes #
Improves README documentation clarity for first-time contributors

Changelog

Changed

  • Clarified yarn install purpose in README to explicitly mention it installs dependencies from package.json
  • Improved README formatting and structure for better readability
  • Fixed version number formatting in package.json (2.0,0 → 2.0.0)

Removed

  • Redundant develop script from package.json (duplicate of dev)
  • Temporary Sass configuration that should be handled by gatsby-theme-carbon

@Subham-KRLX Subham-KRLX requested a review from a team June 13, 2025 04:05
@Subham-KRLX Subham-KRLX requested a review from a team as a code owner June 13, 2025 04:05
@github-actions
Copy link
Contributor

github-actions bot commented Jun 13, 2025 •
edited
Loading

DCO Assistant Lite bot All contributors have signed the DCO.

@Subham-KRLX
Copy link
Contributor Author

Subham-KRLX commented Jun 13, 2025

@carbon-design-system maintainers — I'd love your feedback on this small improvement to the README. I'm a first-time contributor. 🙂

@Subham-KRLX Subham-KRLX force-pushed the improve-readme-install-instruction branch from 470aa00 to 2cb6c6a Compare June 13, 2025 04:24
@Subham-KRLX
Copy link
Contributor Author

Subham-KRLX commented Jun 13, 2025

Hi 👋, first-time contributor here! I’ve signed off the commit to fix the DCO error. Kindly requesting workflow approval and a review when convenient. Thank you! 🙌

@Subham-KRLX
Copy link
Contributor Author

Subham-KRLX commented Jun 13, 2025

I have read the DCO document and I hereby sign the DCO.

@Subham-KRLX Subham-KRLX force-pushed the improve-readme-install-instruction branch from 8ed5d76 to 0e3346b Compare June 13, 2025 10:22
@Subham-KRLX
Copy link
Contributor Author

Subham-KRLX commented Jun 13, 2025

Hi team 👋 — all checks are now passing including the DCO. The Netlify preview is still blocked due to needing workflow approval from a maintainer.

As this is a small docs change and I’m a first-time contributor, I’d really appreciate a quick review and workflow approval when you get a chance. Thank you! 😊

@Subham-KRLX
Copy link
Contributor Author

Subham-KRLX commented Jun 13, 2025

Hi @emyarod @annawen1 @alina-jacob —
All checks have passed (DCO ✅); the build runs locally.
Could one of you please approve the workflow so the Netlify preview can complete? Thanks in advance! 😊

@Subham-KRLX
Copy link
Contributor Author

Subham-KRLX commented Jun 17, 2025

Hi again 👋

Just a gentle follow-up on this PR. All checks are now passing ✅
The only thing pending is workflow approval so the Netlify preview can complete, and a quick review from a code owner.

Happy to make any changes if needed — thanks again for your time! 🙌

annawen1
annawen1 reviewed Jun 17, 2025
View reviewed changes
Copy link
Member

@annawen1 annawen1 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Hi @Subham-KRLX, thanks for taking the time to contribute! Is there a specific issue that was opened for the changes you're making in this PR? Some of the changes like adding the develop script or the yarn install description seem a bit redundant / unnecessary to me.

@Subham-KRLX
Copy link
Contributor Author

Subham-KRLX commented Jun 19, 2025

Hi @annawen1, thanks for your feedback!
This PR was created to improve clarity for first-time contributors (like myself) around the install flow and setup. The additions (like the develop script note and yarn install clarification) were intended to reduce friction during onboarding.
That said, I totally understand if any part feels redundant — I’m happy to trim or revise based on your recommendation. Would you prefer I keep only the install clarification and drop the develop section, or would you like both removed?
Appreciate your time and guidance!

@Subham-KRLX
Copy link
Contributor Author

Subham-KRLX commented Jun 19, 2025

Hi @annawen1 👋

I've updated the README based on your feedback — the install clarification is retained, and the extra parts have been trimmed as discussed.
Let me know if there's anything else you'd like changed. Thanks again for your help and time! 😊

@Subham-KRLX Subham-KRLX requested a review from annawen1 June 19, 2025 02:43
tay1orjones
tay1orjones requested changes Jun 24, 2025
View reviewed changes
@Subham-KRLX
Copy link
Contributor Author

Subham-KRLX commented Jun 26, 2025

@tay1orjones Thanks for the thorough review! I'll:

  1. Revert the Sass config changes (keeping dependencies transient)
  2. Remove the redundant develop script
  3. Restore original wiki link formatting

Will push these updates shortly. Appreciate your guidance on maintaining project standards!

@Subham-KRLX Subham-KRLX requested a review from tay1orjones June 26, 2025 03:30
Subham-KRLX added a commit to Subham-KRLX/carbon-website that referenced this pull request Jul 9, 2025
@Subham-KRLX
docs: complete PR carbon-design-system#4629 updates per review
3d25a27 
- Improved README formatting and clarity
- Removed Sass config from gatsby-config.js
- Cleaned package.json (removed redundant scripts and Sass deps)
- Fixed version number formatting
@Subham-KRLX
Copy link
Contributor Author

Subham-KRLX commented Jul 9, 2025

@tay1orjones
Could someone with deploy permissions please trigger a Netlify build for this PR?
All changes are complete and checks are passing.

@Subham-KRLX
Copy link
Contributor Author

Subham-KRLX commented Jul 12, 2025

hey,@alisonjoseph
My PR (#4629) is hitting build errors:
Webpack fails – Can’t resolve @carbon/react
Plugin error – gatsby-plugin-remove-serviceworker won’t load
Locally, it works after reverting non-README changes. Could someone:
✅ Approve the workflow to unblock Netlify?
✅ Confirm if @carbon/react needs explicit install?

All DCO checks pass. Let me know how to fix.

@Subham-KRLX Subham-KRLX force-pushed the improve-readme-install-instruction branch from fcf738a to df01bd7 Compare July 17, 2025 10:47
tay1orjones
tay1orjones requested changes Jul 28, 2025
View reviewed changes
Copy link
Member

@tay1orjones tay1orjones left a comment

Choose a reason for hiding this comment

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

I'm still seeing gatsby-plugin-sass, path-browserify, and sass listed as hard deps. Removing those and running yarn install will update the yarn.lock.

Deploy previews only run for maintainers with write access to the repo for security reasons, but we can test changes locally still.

gatsby-node.js
Comment on lines +25 to +36
// Explicitly resolve @carbon packages to their node_modules path
'@carbon/react': path.resolve(__dirname, 'node_modules/@carbon/react'),
'@carbon/icons-react': path.resolve(__dirname, 'node_modules/@carbon/icons-react'),
'@carbon/styles': path.resolve(__dirname, 'node_modules/@carbon/styles'), // Keep for Sass if needed
},
// Fallback for older module resolution patterns
fallback: {
'crypto': false, // Example of adding fallbacks if needed for older modules
'stream': false,
'path': require.resolve('path-browserify'), // Ensure path resolves correctly
// You might need more if other 'cannot resolve' errors appear for node built-ins
}
Copy link
Member

@tay1orjones tay1orjones Jul 28, 2025

Choose a reason for hiding this comment

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

What is this for?

gatsby-node.js
},
});

// Disable sourcemaps in production
Copy link
Member

@tay1orjones tay1orjones Jul 28, 2025

Choose a reason for hiding this comment

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

imo this is valuable as devtool: false doesn't really convey relation to sourcemaps

README.md
Comment on lines -48 to -50
```
yarn build
```
Copy link
Member

@tay1orjones tay1orjones Jul 28, 2025

Choose a reason for hiding this comment

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

Why remove code formatting for this?

README.md
Comment on lines -14 to 16

```
src
├── components
Copy link
Member

@tay1orjones tay1orjones Jul 28, 2025

Choose a reason for hiding this comment

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

removing the backtics cause this to not be formatted properly. I'd just leave them in.

@tay1orjones
Copy link
Member

tay1orjones commented Jul 28, 2025

Also if you run yarn format before pushing up your changes it'll autoformat everything for you and resolve some of these unecessary diffs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tay1orjones tay1orjones tay1orjones requested changes

@emyarod emyarod Awaiting requested review from emyarod

@alina-jacob alina-jacob Awaiting requested review from alina-jacob alina-jacob is a code owner automatically assigned from carbon-design-system/design

@annawen1 annawen1 Awaiting requested review from annawen1

Requested changes must be addressed to merge this pull request.

Assignees

No one assigned

Labels

status: waiting for author's response 💬

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@Subham-KRLX @tay1orjones @annawen1 @alina-jacob