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

chore: modify readme about how we want people to acquire Blockly #7661

Merged
merged 2 commits into from
Dec 1, 2023

Conversation

BeksOmega
Copy link
Collaborator

The basics

The details

Resolves

Fixes #7624

Proposed Changes

Modifies the README to hopefully be correct about how we want people to import things.

Reason for Changes

Getting our code is important.

Test Coverage

No functional changes!

Documentation

This is documentation silly!

Additional Information

N/A

@BeksOmega BeksOmega requested a review from a team as a code owner November 21, 2023 19:52
@github-actions github-actions bot added the PR: chore General chores (dependencies, typos, etc) label Nov 21, 2023
scripts/package/README.md Outdated Show resolved Hide resolved

If you need more flexibility, you'll want to define your imports more carefully:
### unpkg
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it not be better to put this after the npm section—or even omit it entirely?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Contributor

@cpcallen cpcallen left a comment

Choose a reason for hiding this comment

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

Two questions:

  • Is it necessary to provide these instructions in package.json? Could we not just refer people to devsite? Or is the idea to have old versions of the NPM package have instructions that are correct for those versions?

  • Could we not eliminate scripts/package/README.md by combining it with the top-level README.md? It's great that you're updating it here but I get the impression it is often overlooked and left with out-of-date instructions. That's slightly less likely to happen with the top-level README.md, and certainly having only one copy seems otherwise preferable.

scripts/package/README.md Show resolved Hide resolved
scripts/package/README.md Outdated Show resolved Hide resolved
@BeksOmega
Copy link
Collaborator Author

  • Is it necessary to provide these instructions in package.json? Could we not just refer people to devsite? Or is the idea to have old versions of the NPM package have instructions that are correct for those versions?

I think most folks provide basic usage instructions with their package. It seems like a more use-friendly experience if they can at least load the code from the info on the readme. But pointing to devsite would be more maintainble, so I wouldn't mind just including al ink and no instructions.

  • Could we not eliminate scripts/package/README.md by combining it with the top-level README.md? It's great that you're updating it here but I get the impression it is often overlooked and left with out-of-date instructions. That's slightly less likely to happen with the top-level README.md, and certainly having only one copy seems otherwise preferable.

The root-level readme talks a lot about our release cycle and things like that. I don't think it makes sense to include that here, but can be nice for people that want to contribute to the project. I would rather just add a link to devsite than include info in the top-level readme.

@maribethb
Copy link
Contributor

I agree that it makes sense to keep this readme separate from the top-level one, however:

  • It is easy for this to get out of date since it's not next to our other documentation.
  • You still can't do anything with the package unless you also explain Blockly.inject, so as comprehensive as these are, it doesn't explain enough to start using blockly.
  • All of this information should also be somewhere on devsite if it isn't already (the "getting the code" page probably needs to be updated and should probably contain very similar content).
  • This page needs to link to devsite to point people at the documentation.

So given the above, I think we should either:

  • put the bare minimum here (npm install blockly) and link to devsite for the rest
  • update the devsite "get the code" page to be identical content to this and add a comment on the devsite page to remind authors to update this when they update that page

Thoughts?

@maribethb
Copy link
Contributor

Also meta question: Are we not recommending people use import * as Blockly from 'blockly' anymore? That's what the sample app does. They don't need to call setLocale if they do that.

@BeksOmega
Copy link
Collaborator Author

update the devsite "get the code" page to be identical content to this and add a comment on the devsite page to remind authors to update this when they update that page

This PR updates things to match the new get the code page (cl/577317152). I'll add a comment to tell people to update this when they update that.

Also meta question: Are we not recommending people use import * as Blockly from 'blockly' anymore? That's what the sample app does. They don't need to call setLocale if they do that.

I'm not recommending that because it doesn't work great "batteries included" anymore since we changed how the javascript generators are exported.

I think it's clearer to just tell people to import all of the things they need individually.

Copy link
Contributor

@cpcallen cpcallen left a comment

Choose a reason for hiding this comment

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

I tend to agree that it would be better just to point to devsite (for maintenance reasons) but otherwise this looks fine.

@BeksOmega BeksOmega merged commit f363252 into google:develop Dec 1, 2023
8 checks passed
@BeksOmega BeksOmega deleted the chore/readme branch May 14, 2024 16:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: chore General chores (dependencies, typos, etc)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Docs say blockly bundle comes with JS generator
3 participants