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

use ApexCharts instead of HighCharts #117

Merged
merged 6 commits into from
Nov 27, 2024

Conversation

vol1ura
Copy link
Contributor

@vol1ura vol1ura commented Nov 25, 2024

Highcharts license is not free, and there are problems (cors) with loading of the library, so I suggest using Apexcharts, and adding js file to assets as other js.

There is a small difference in the set of options, I tried to keep the previous style of charts.

image image

@@ -6,9 +6,6 @@
<title>Rails Performance</title>
<%= csrf_meta_tags %>
<%= csp_meta_tag if ::Rails::VERSION::STRING.to_f >= 5.2 %>
<script>
window.APP_TIME_ZONE = '<%= Rails.application.config.time_zone.presence || 'UTC' %>';
Copy link
Owner

Choose a reason for hiding this comment

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

can we keep it and implement something like it?

Copy link
Owner

Choose a reason for hiding this comment

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

@vol1ura maybe some other options can be considered

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here I try to use option datetimeUTC: false in charts options. It seems the time is displayed correctly now. Please, check

Copy link
Owner

Choose a reason for hiding this comment

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

hmm, maybe it works fine

could you please check if you have the same. I just see some inconsistency and don't know why

image

Copy link
Owner

Choose a reason for hiding this comment

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

also not sure about this part of the tooltip, I think it's obvious anyway

image

also not sure but I have a feeling this chart library works slower on mouse hover

but I like the tooltip with time on X-axis

Copy link
Contributor Author

Choose a reason for hiding this comment

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

also not sure about this part of the tooltip, I think it's obvious anyway

Tooltip title can be disabled on all charts, I'll add this param

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just see some inconsistency and don't know why

I've fixed this issue. Last right label on X axis sometimes doesn't fit, I don't know what to do about it.

image

Copy link
Owner

Choose a reason for hiding this comment

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

@vol1ura please also move Y axis to the left side. Basically, make a chart very similar to the current version.

After that, I think I can try to release "alpha" version and try it on production

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@igorkasyanchuk ok, done!

Copy link
Owner

Choose a reason for hiding this comment

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

released as

Successfully registered gem: rails_performance (1.4.1.alpha1)

you can try to upgrade.
I also will try to run in on production

@igorkasyanchuk igorkasyanchuk merged commit 6ba24bd into igorkasyanchuk:master Nov 27, 2024
@igorkasyanchuk
Copy link
Owner

PR merged, I think it works fine
thanks!

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.

2 participants