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

Updated docs (why Chart.js + getting started + step-by-step guide) #10816

Merged
merged 9 commits into from
Nov 11, 2022

Conversation

igorlukanin
Copy link
Member

@igorlukanin igorlukanin commented Oct 20, 2022

This PR reworks a few things in the Chart.js docs:

  • Adds convenient links to the main page and the getting started page to help newcomers navigate through the docs.
  • Adds a very elaborate "Why Chart.js" section to the main page to help explain the advantages of Chart.js and convince new users to try Chart.js. (Not sure if a link to other charting libraries should stay there. I think we can't pretend they don't exist but I'm open to opinions here.)
  • Deduplicates nearly identical code examples at the main page, the getting started page, and the usage page. Now, the code example is located on the getting started page only.
  • Introduces a lengthy and elaborate step-by-step guide to help new Chart.js users to get familiar with all major concepts: chart types and elements, datasets, customization, plugins, components, etc. @dangreen and @Arantiryo helped check this guide for sanity but it would be great to hear feedback from a wider community.
  • Fixes typos and styles here and there.

This PR is work-in-progress. Known issues:

  • In the step-by-step guide, in the package.json file, Chart.js version needs to be updated to ^4.0.0 before the merge.

Also, it resolves #10620.

A few screenshots:
Screenshot 2022-10-20 at 22 43 23
Screenshot 2022-10-20 at 22 43 28
Screenshot 2022-10-20 at 22 43 32

@etimberg @kurkle @LeeLenaleee and all—I'd love to know what you think!

@igorlukanin igorlukanin marked this pull request as draft October 20, 2022 19:00
Copy link
Collaborator

@LeeLenaleee LeeLenaleee left a comment

Choose a reason for hiding this comment

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

Some comments, am on phone so might have missed things

docs/developers/index.md Outdated Show resolved Hide resolved
docs/getting-started/index.md Outdated Show resolved Hide resolved
docs/getting-started/index.md Outdated Show resolved Hide resolved
docs/getting-started/index.md Outdated Show resolved Hide resolved
docs/getting-started/index.md Outdated Show resolved Hide resolved

* **[Get started with Chart.js](./getting-started/) — best if you're new to Chart.js**
* Migrate from [Chart.js v3](./migration/v4-migration.html) or [Chart.js v2](./migration/v3-migration.html)
* Join the community on [Slack](https://chartjs-slack.herokuapp.com/) and [Twitter](https://twitter.com/chartjs)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would keep the Twitter handle away, did not know there was one and it seems to be inactive.

Rather have people to go to slack which in my opinion is clearer and easter for multiple people to react

Copy link
Member

Choose a reason for hiding this comment

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

@etimberg has access to that Twitter account, right? I think we have promote releases there, so I'd be fine having it here.

Copy link
Member Author

@igorlukanin igorlukanin Oct 21, 2022

Choose a reason for hiding this comment

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

@LeeLenaleee Great that you've spotted this! I wanted to start a conversation about the Twitter account 😄

Indeed, there hasn't been a lot of activity on https://twitter.com/chartjs in the last ~18 months. At the same time, Slack and Twitter support quite different scenarios: the former is more a community comms platform while the latter is more a news channel.

I believe that we can try reviving Chart.js account on Twitter and maybe grow it to the audience size of https://twitter.com/d3js_org 🤷

I can volunteer to take care of it. @etimberg, WDYT?

As the very first step, I would suggest:

  • posting major/minor release updates
  • posting highlights about important and/or actively discussed PRs/issues
  • posting links to interesting GitHub Discussions/StackOverflow questions
  • re-posting other tweets mentioning Chart.js

The timing is great because Chart.js v4 is nearing. We can try make a buzz about it.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I have the twitter login. Happy to share access since I don't do much with it. We get tagged by someone else every few days, but it's mostly folks showing off projects that they've built

Copy link
Member

@kurkle kurkle left a comment

Choose a reason for hiding this comment

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

It would be nice to have some "brand colors" in these usage images, no?


* **[Get started with Chart.js](./getting-started/) — best if you're new to Chart.js**
* Migrate from [Chart.js v3](./migration/v4-migration.html) or [Chart.js v2](./migration/v3-migration.html)
* Join the community on [Slack](https://chartjs-slack.herokuapp.com/) and [Twitter](https://twitter.com/chartjs)
Copy link
Member

Choose a reason for hiding this comment

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

@etimberg has access to that Twitter account, right? I think we have promote releases there, so I'd be fine having it here.

docs/samples/information.md Outdated Show resolved Hide resolved

* **[Get started with Chart.js](./getting-started/) — best if you're new to Chart.js**
* Migrate from [Chart.js v3](./migration/v4-migration.html) or [Chart.js v2](./migration/v3-migration.html)
* Join the community on [Slack](https://chartjs-slack.herokuapp.com/) and [Twitter](https://twitter.com/chartjs)
Copy link
Member

Choose a reason for hiding this comment

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

re slack, I wonder if we should consider migrating from Slack to Discord

@igorlukanin
Copy link
Member Author

I believe I've accounted for all comments in the thread (apart from Twitter/Slack/Discord which I think we can discuss separately). Now this PR's ready for the final review 😄

@igorlukanin igorlukanin marked this pull request as ready for review October 25, 2022 15:01
@igorlukanin igorlukanin changed the title WIP: Updated docs (why Chart.js + getting started + step-by-step guide) Updated docs (why Chart.js + getting started + step-by-step guide) Oct 25, 2022
@@ -0,0 +1,28 @@
<div>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why are you adding these files in the public folder of vuepress?

Same goes for all the files and folders you added in the usage folder.

I think its better to just integrete these kind of examples in the docs pages itself (md files) and use place a chart chart custom chart undeaneath it with that config.

Maby we can look into the plugin to add them so we can disable the user input so the user only sees the chart

Copy link
Member Author

@igorlukanin igorlukanin Oct 26, 2022

Choose a reason for hiding this comment

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

@LeeLenaleee Cool question! So, these public files represent self-contained, pre-built Chart.js examples that I embed into the step-by-step guide. If you'd like to, consider running pnpm docs:dev and checking out http://localhost:8080/docs/master/getting-started/; you might enjoy it 😄

Some of these examples rely on an external dependency to fetch substantially sized "real-world data," and embedding the library in the docs would be unreasonable. I've explored embedding the datasets directly, but they are dozens of KBs, so it's also not an option.

Maby we can look into the plugin to add them so we can disable the user input, so the user only sees the chart

Yep, it's a viable option for the file you're commenting (demo.html). I've submitted a PR to vuepress-theme-chartjs that would allow that: simonbrunel/vuepress-theme-chartjs#4

I think its better to just integrete these kind of examples in the docs pages itself (md files)

Right. With the PR above, that one can be replaced with something like this:

```js chart-editor
const data = [
  { year: 2010, count: 10 },
  { year: 2011, count: 20 },
  { year: 2012, count: 15 },
  { year: 2013, count: 25 },
  { year: 2014, count: 22 },
  { year: 2015, count: 30 },
  { year: 2016, count: 28 },
];

module.exports = {
  config: {
    type: 'bar',
    data: {
      labels: ['Red', 'Blue', 'Yellow', 'Green', 'Purple', 'Orange'],
      datasets: [{
        label: '# of Votes',
        data: [12, 19, 3, 5, 2, 3],
        borderWidth: 1
      }]
    },
    options: {
      scales: {
        y: {
          beginAtZero: true
        }
      }
    }
  },
  chartOnly: true
};

Copy link
Member Author

Choose a reason for hiding this comment

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

Let's wait for @simonbrunel to merge the PR to vuepress-theme-chartjs. Alternatively, I can submit a follow-up PR later after the theme is updated so this one is not blocked.

Copy link
Collaborator

@LeeLenaleee LeeLenaleee Oct 26, 2022

Choose a reason for hiding this comment

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

While I appreciate the work I am against merging this for the following reasons:

  1. I find it a security risk, those js files are big and hard to read so difficult to know if you putted something in there that's not supposed to be there.

  2. Its a maintenance burden to update them Everytime something in the codebase has changed

  3. Sample in the docs should be minimalistic in my eyes and not rely on external data which can change or go down. If we want an example with loads of data we should add it in the samples section with generated data.

So instead of embedding these kind of charts, just show simple charts and in the way we do it elsware in the docs with Simons plugin

Copy link
Member

Choose a reason for hiding this comment

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

I fully agree with @LeeLenaleee, especially on the maintenance part. There should be no pre-built files in the repository. I'm not even sure how the usage/* files are supposed to be generated? where are the sources? IMO, embedding complex examples in the docs should go through some services like CodePen or alternatives, so users have access to the entire source code.

But as a getting started, I would indeed expect something minimalistic as mentioned by @LeeLenaleee.

Copy link
Member Author

Choose a reason for hiding this comment

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

@LeeLenaleee @simonbrunel This is excellent feedback; thanks! Honestly, I dislike the current state of things just like you do. It was an attempt to have interactive charts rather than images, but it looks ugly now. Let me roll this PR back to just images and remove all the bloat.

@simonbrunel has a great point on CodePen/CodeSandbox/etc. However, they would still be external to docs and could potentially break, so let's skip that options for now.

Copy link
Member Author

Choose a reason for hiding this comment

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

Rolled back to images. (No new files under .vuepress anymore.) IMO, much better now. WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

Images make sense to me. A CodePen or similar could work, but the maintenance burden is there. Maybe there are ways we could automatically update the codepen during deploys?

etimberg
etimberg previously approved these changes Oct 27, 2022
Copy link
Collaborator

@LeeLenaleee LeeLenaleee left a comment

Choose a reason for hiding this comment

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

image
White space seems excessive to me, can you cut that out?

@igorlukanin
Copy link
Member Author

@LeeLenaleee Good catch! I probably should've elaborated on this earlier. I purposefully made all usage-* images in this PR have the same width for a couple of reasons: they are consistent in size with each other and closely resemble what the end user would get when following the guide on their own. It surely might look strange in the PR, but I really think it makes sense and looks good on the page. (I'm open to changes, really, but I hope that my explanation makes sense.)

@LeeLenaleee
Copy link
Collaborator

Will give it a look when I have time to access my computer tomorrow evening

@igorlukanin
Copy link
Member Author

@etimberg Could you please mark this for v4? I've checked with @LeeLenaleee and it looks like it might take a day or two to review this.

@etimberg etimberg added this to the Version 4.0 milestone Nov 1, 2022
@etimberg
Copy link
Member

etimberg commented Nov 1, 2022

@igorlukanin done 😄

Copy link
Collaborator

@LeeLenaleee LeeLenaleee left a comment

Choose a reason for hiding this comment

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

Looks verry promosing, couple of nits, keep in mind english is not my native tongue but this sounded better to me.

Also maby I am blind but I don't see the images used except for the basic bar chart is that correct?

Still not a big fan of the images, but that is something we can easily change later so that is good for now 👍🏻

docs/index.md Outdated Show resolved Hide resolved
docs/index.md Outdated Show resolved Hide resolved
docs/index.md Outdated Show resolved Hide resolved
docs/index.md Outdated Show resolved Hide resolved
docs/index.md Outdated Show resolved Hide resolved
docs/getting-started/index.md Show resolved Hide resolved
Co-authored-by: Jacco van den Berg <jaccoberg2281@gmail.com>
@igorlukanin
Copy link
Member Author

@LeeLenaleee Thanks a lot for your review, I've applied almost all your suggestions!

Looks ready now? Cc @etimberg

@etimberg etimberg merged commit 6917584 into chartjs:master Nov 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Chart.js advantages and prominent features explained
5 participants