chartjs / chartjs-adapter-luxon

Luxon adapter for Chart.js
MIT License
33 stars 24 forks source link

displayFormats unexpected result #46

Closed ocarreterom closed 1 year ago

ocarreterom commented 2 years ago

Hi! I'm trying to display the weekday but the result is unexpected.

I'm not sure if it's an error with this plugin, with Chartjs or I'm doing something wrong.

https://codesandbox.io/s/jovial-hermann-pnb72?file=/src/index.js

I expect to get the label monday, tuesday but I get monday, 29 nov, tuesday, 30 nov

import { Chart, registerables } from "chart.js";
import "chartjs-adapter-luxon";

Chart.register(...registerables);

const canvas = document.getElementById("chart");

const chart = new Chart(canvas, {
  type: "bar",
  data: {
    datasets: [
      {
        data: [
          { x: "1", y: 10 }, // monday
          { x: "2", y: 10 } // tuesday
        ]
      }
    ]
  },
  options: {
    scales: {
      x: {
        type: "time",
        time: {
          unit: "day",
          parser: "E",
          displayFormats: {
            day: { weekday: "long" } // expected monday/tuesday
          }
        }
      }
    }
  }
});

Thanks!

kurkle commented 2 years ago

I thinks you need to use the tokens.

cccc | EEEE | day of the week, as an unabbreviated localized string | Wednesday

stockiNail commented 2 years ago

@ocarreterom I think the displayFormats config is wrong.

The displayFormats object can contain values where the key is the unit and the value is the format. Inner nodes (like day: { weekday: "long" }) are not enabled.

Try and let us know if the following works:

displayFormats: {
  day: "EEEE"
 }
ocarreterom commented 2 years ago

@kurkle yes, that works but the result is not localized.

@stockiNail I'm using an object instead because I'm trying to run this line https://github.com/chartjs/chartjs-adapter-luxon/blob/master/src/index.js#L60 and get the localized string using Intl.

If I set an invalid format like { day: { weekday: 'asdf' } } I get this error: weekday must be "narrow", "short", or "long", so the code is running.

It seems that the custom format is merging with this default format.

ocarreterom commented 2 years ago

@kurkle The Luxon documentation says https://moment.github.io/luxon/#/formatting?id=intl-1

Note toFormat defaults to en-US. If you need the string to be internationalized, you need to set the locale explicitly like in the example above (or more preferably, use toLocaleString).

And this plugin is not setting the locale explicitly.

stockiNail commented 2 years ago

If I set an invalid format like { day: { weekday: 'asdf' } } I get this error: weekday must be "narrow", "short", or "long", so the code is running.

I meant you should use displayFormats: { day: "EEEE" }. I have tried in your sample and seems working.

And this plugin is not setting the locale explicitly.

To set the locale information, you should use adapters.date nodes in the scale options.

See https://github.com/chartjs/chartjs-adapter-luxon/pull/42 where it's explained which options you can set.

See also this codepen, when a locale is set. https://codepen.io/stockinail/pen/rNyYapz

Config:

scales: {
  x: {
    type: 'time', 
    ...
    adapters: {
      date: {
        locale: 'en'
      }
    },
    ...
  }
}
stockiNail commented 2 years ago

@ocarreterom Using your sandbox.

Options

options

Result

locale

stockiNail commented 2 years ago

I didn't recognize you used locale: 'es'

Here is the result with locale 'es':

Screenshot 2021-12-01 165938 locale

ocarreterom commented 2 years ago

@stockiNail that works!

It's a little verbose to set the locale two times and I still think that the Intl notation should work.

Thanks for your help!

stockiNail commented 2 years ago

@ocarreterom Agree! See my open issue: https://github.com/chartjs/Chart.js/issues/7859. Planned for CHART.JS version 4

stockiNail commented 1 year ago

@stockiNail that works!

It's a little verbose to set the locale two times and I still think that the Intl notation should work.

Thanks for your help!

@ocarreterom with version 1.2.0 of this adapter and Chart.js > 3.9.0, the adapter will use the locale option at chart level if not specified in the adapter.date options.

In this way, you can set the locale only at chart options level. I think this could be closed, doesn't it?

ocarreterom commented 1 year ago

@stockiNail yes, this can be close. Thanks!