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

Make plugin no longer self registering #24

Merged
merged 2 commits into from
Feb 3, 2022

Conversation

LeeLenaleee
Copy link
Contributor

@simonbrunel simonbrunel added this to the Version 2.0 milestone Jan 30, 2022
Copy link
Member

@simonbrunel simonbrunel left a comment

Choose a reason for hiding this comment

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

Awesome! Strange that you didn't have to change the plugin integration in the docs. Not sure why it was already integrated that way.

Can you also add a migration page, similar to this one? (mostly a copy/paste and remove parts that doesn't apply to this plugin).

docs/guide/getting-started.md Outdated Show resolved Hide resolved
docs/guide/getting-started.md Outdated Show resolved Hide resolved
docs/guide/getting-started.md Outdated Show resolved Hide resolved
Copy link
Member

@simonbrunel simonbrunel left a comment

Choose a reason for hiding this comment

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

Thanks a lot @LeeLenaleee!

@simonbrunel simonbrunel merged commit da48ed9 into chartjs:master Feb 3, 2022
@gregh3269
Copy link

Hello,
I have retested this and now for me it does not load deferred. I use the html version, and at the moment I am using

new Chart(..
plugins: {
            deferred: {
              xOffset: 150,   ## defer until 150px of the canvas width are inside the viewport
              yOffset: '50%', ## defer until 50% of the canvas height are inside the viewport
              delay: 500      ## delay of 500 ms after the canvas is considered inside the viewport
            }
          }
...

as in the read me. Do I have to change anything here?

The docs now mention this:

var chart = new Chart(ctx, {
  plugins: [ChartDeferred],
  options: {
    // ...
  }
})

@LeeLenaleee
Copy link
Contributor Author

@gregh3269 do you have a reproducible sample because when I tested this it worked fine: https://codepen.io/leelenaleee/pen/ExwBJgK

@gregh3269
Copy link

gregh3269 commented Feb 3, 2022

Ok, I can see we now must have a

Chart.register(ChartDeferred);

first, which makes a global, guess we now cannot do it inline ?

from the getting started what does this mean/do then?

OR only to specific charts

var chart = new Chart(ctx, {
  plugins: [ChartDeferred],
  options: {
    // ...
  }
})

..I can see from here
https://www.chartjs.org/docs/latest/developers/plugins.html
we need the global.

Might be worth updating the README.md also for this change for the tldr's. ;-)

Thanks.

@LeeLenaleee
Copy link
Contributor Author

@gregh3269 mb, seems I forgot to save the pen, updated it with inline register and it seems to work fine so if it is not working please share a sample of it: https://codepen.io/leelenaleee/pen/ExwBJgK

@gregh3269
Copy link

ok thanks, makes sense now!

new Chart(...
  plugins: [ChartDeferred],
  ...
  options: {
    ...
    plugins: {
      deferred: {
        xOffset: 150,   ## defer until 150px of the canvas width are inside the viewport
        yOffset: '50%', ## defer until 50% of the canvas height are inside the viewport
        delay: 500      ## delay of 500 ms after the canvas is considered inside the viewport
      }
    }
  }
});

@LeeLenaleee LeeLenaleee deleted the feature/remove-auto-import branch February 3, 2022 11:48
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.

3 participants