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 date and time formats configurable through options #112

Closed
wants to merge 1 commit into from

Conversation

rept
Copy link

@rept rept commented Aug 24, 2019

Hi @ankane

This is with regards to ankane/chartkick#445

I've added two options:

  • date_format
  • time_format

And kept the default values in case they aren't being used.

I'm not sure if the naming is clear like this. Happy to hear your feedback.

Hi @ankane

This is with regards to ankane/chartkick#445

I've added two options:

- date_format
- time_format

And kept the default values in case they aren't being used.

I'm not sure if the naming is clear like this. Happy to hear your feedback.
@ankane
Copy link
Owner

ankane commented Sep 4, 2019

Hey @rept, thanks for the PR 👍 I think it'd be good to have a single option for setting the format regardless of the time unit. How about time_format / timeFormat (we'd want to allow both since some languages use camel case and others use underscore)?

@rept
Copy link
Author

rept commented Sep 4, 2019

@ankane well my first thought was indeed creating a single option. However in the code the difference is made based on

hour || timeDiff > 0.5

How do you see a single option in that case?

@ankane
Copy link
Owner

ankane commented Sep 4, 2019

A single option should work if you replace date_format with time_format. However, the PR currently only sets options.scales.xAxes[0].time.displayFormats for two conditions. It should set it anytime time_format is set.

@rept
Copy link
Author

rept commented Sep 4, 2019

Sorry @ankane I'm not seeing how you can get both into time_format.

In one case we have:
"MMM D, h a"
in the other we have
"h:mm a"

How can you put both in one time_format string? I believe my naming isn't clear enough like I said in the first post. Maybe that's the confusing part.

About the other places where time_format should be set. I've searched the complete code but only found the string 'MMM D, h a' and 'h:mm a' in these 2 locations.

@ankane
Copy link
Owner

ankane commented Sep 4, 2019

@rept If the user passes time_format: "MMM" to Chartkick, it should set options.scales.xAxes[0].time.displayFormats and options.scales.xAxes[0].time.tooltipFormat to that format.

@ankane
Copy link
Owner

ankane commented Nov 10, 2019

Cleaning up stale PRs

@ankane ankane closed this Nov 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants