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

feat: pass some chart options to DateAdapter #10528

Merged
merged 2 commits into from Jul 30, 2022
Merged

feat: pass some chart options to DateAdapter #10528

merged 2 commits into from Jul 30, 2022

Conversation

dangreen
Copy link
Collaborator

@dangreen dangreen marked this pull request as ready for review July 29, 2022 10:54
test/specs/scale.time.tests.js Outdated Show resolved Hide resolved
src/core/core.adapters.js Outdated Show resolved Hide resolved
@simonbrunel
Copy link
Member

@kurkle it has been a very long time I didn't look at Chart.js, so not sure how my review will be valuable 😄

I'm not sure I like the suggested way to pass down "some" of the chart options to the adapters. I would probably make no assumption of what adapters need from the chart options and pass the entire chart option hierarchy. Adapters can them fallback their options on whatever other chart options they want. This is actually similar to the scale init() method.

Also, I'm not a fan of storing this chartOptions as part of the DateAdapter. If adapters can override the constructor, I would just pass chartOptions as second argument, else have an init() method that takes this chartOptions as unique argument, so adapters can initialize their options with defaults.

_adapters._date.override({
  _id: 'luxon',

  // Not sure that override works!
  constructor(options: LuxonAdapterOptions, { locale }: ChartOptions) {
    this.options = { locale, ...options };
  }
})

// else

_adapters._date.override({
  _id: 'luxon',

  // New adapter API
  init({ locale }: ChartOptions) {
    if (!this.options.locale) {
       this.options.locale = locale;
    }
  }
})

Thoughts?

@stockiNail
Copy link
Contributor

@simonbrunel I agree with you and I think the init() methods could be the better solution (but that's my personal opinion). But the default init() shouldn't throw any exception (like all the others) but it should be empty.

@stockiNail
Copy link
Contributor

stockiNail commented Jul 29, 2022

@dangreen thank you very much because you anticipated me! I was waiting for version 4.0 (as tagged in the issue) ! 👍

@simonbrunel
Copy link
Member

@stockiNail Agreed, init() seems better because adapters can't call the base constructor with super(). I also agree that init() should not be abstract and the base implementation should be empty.

@kurkle
Copy link
Member

kurkle commented Jul 29, 2022

@simonbrunel I just knew you'd have some good points, especially when talking interfaces, thanks!

init sounds good to me, with a default implementation. My initial thought was to resolve the locale to the adapter options, but as the values are incompatible between adapters, that would not work well.

I'd tend pass just the locale and maybe core version.
I can't think of anything else in the chart options that should affect date adapter behavior.

Edit: to be clear, I'm not against passing the whole chart options.

@dangreen
Copy link
Collaborator Author

Changed it to init method.

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.

Lgtm

@dangreen
Copy link
Collaborator Author

@kurkle @LeeLenaleee Can we move it to v3.9? As is does not contain breaking changes.

@simonbrunel
Copy link
Member

@kurkle I'd tend pass just the locale and maybe core version.

That was a suggestion. I'm also fine to pass only { locale } to the init() method since locale is at the root of the chart options. If later there is a need for more options, we can pass the entire chart options which would simply extends { locale }, so no breaking change. However, I would not pass an object which has a different structure than the chart options and would cherry pick a few (potentially flatten) options based on use cases.

Whatever you guys think is better!

@LeeLenaleee
Copy link
Collaborator

LeeLenaleee commented Jul 29, 2022

@kurkle @LeeLenaleee Can we move it to v3.9? As is does not contain breaking changes.

It was already on 3.9.0 milestone :)

@dangreen
Copy link
Collaborator Author

@LeeLenaleee issue was in v4

@LeeLenaleee LeeLenaleee linked an issue Jul 29, 2022 that may be closed by this pull request
Copy link
Member

@etimberg etimberg left a comment

Choose a reason for hiding this comment

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

Just catching up on the discussion. Looks good to me 👍
I like the idea of passing all the chart options since it hopefully removes the need for future work later

@etimberg etimberg merged commit 844270e into chartjs:master Jul 30, 2022
@dangreen dangreen deleted the feat-date-adapter-chart-options branch August 16, 2022 07:54
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.

Use options.locale property as default for the date adapters
6 participants