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

Zoom page content #20

Merged
merged 1 commit into from
Dec 4, 2024
Merged

Conversation

intermittentnrg
Copy link
Collaborator

I originally sent ctrl+ to browser to zoom, but the screenshot unzooms so I found this method instead.

window size and zoom factor should be provided as options obviously...

Copy link
Contributor

@amotl amotl left a comment

Choose a reason for hiding this comment

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

Excellent. I've not run the program yet, to get an idea about the outcome, but I like your attention to details.

Shall I support you on bringing in the zoom factor as option/setting?

@intermittentnrg
Copy link
Collaborator Author

intermittentnrg commented Oct 17, 2023

Yes support would be fantastic thank you!

I've had a lot of feedback that people couldn't read the text when posting widescreen videos on X/Twitter, and given the styling/studio settings do not work this is my solution which works quite well. I'm not even sure of the expected result from the styling as I've not seen it work...


  1. An outstanding issue from feedback is that the labels on the X axis jump around. Just changing dateformat probably improves it, it annoyingly can only be set globally via grafana.ini and it messes up my normal use of grafana.

  2. I also hacked timeutils.py to suit my needs which also needs to be made an option.

def get_freq_delta(every: str) -> RecurrenceInfo:
    rr_interval = 4
    rr_freq = HOURLY
    delta = timedelta(days=30) # - timedelta(seconds=1)
    return RecurrenceInfo(every=every, frequency=rr_freq, interval=rr_interval, duration=delta)

Not sure your usecase was for timeseries panels? Or I may have misunderstood the options. But this was ideal for me to set step size and time range shown.

@amotl amotl self-assigned this Nov 6, 2023
@amotl amotl removed their assignment May 8, 2024
@amotl
Copy link
Contributor

amotl commented Dec 3, 2024

Hi @maurerle,

it looks like @intermittentnrg also contributed more valuable improvements here, both within the PR, and possibly also about the get_freq_delta function? If you are still in the right mood to improve grafanimate further, would you be keen on wrapping this up?

With kind regards,
Andreas.

@maurerle
Copy link
Collaborator

maurerle commented Dec 3, 2024

Hi @amotl
Thanks for taking care of the release!
I am currently quite busy with other things, but if I have some more free time again and needs to create new graphics, I will pursue this further :)

But I am more than happy that we got something working with latest grafana for now :)
I did not have the need to zoom panels with grafana yet, so I am still unsure if we should add yet another config param for this

Copy link

codecov bot commented Dec 3, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.87%. Comparing base (01dfd7c) to head (2ade270).
Report is 5 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main      #20   +/-   ##
=======================================
  Coverage   94.87%   94.87%           
=======================================
  Files           5        5           
  Lines         390      390           
=======================================
  Hits          370      370           
  Misses         20       20           
Flag Coverage Δ
unittests 94.87% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@amotl amotl force-pushed the zoom-content branch 2 times, most recently from cf385e4 to b48944a Compare December 3, 2024 23:39
I originally sent ctrl+ to browser to zoom, but the screenshot un-zooms
so I found this method instead.
Copy link
Contributor

@amotl amotl left a comment

Choose a reason for hiding this comment

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

Hi @intermittentnrg,

I've extended your patch to make relevant new options configurable using --window-size and --zoom-factor, and added one question about its ingredients.

With kind regards,
Andreas.

Comment on lines -41 to +49
self.set_window_size(1920, 1080)

if self.window_size:
self.set_window_size(
int(self.window_size[0]),
int(self.window_size[1] + (85 * self.zoom_factor)),
)
Copy link
Contributor

@amotl amotl Dec 3, 2024

Choose a reason for hiding this comment

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

Is that height + 85 * self.zoom_factor formula correct? Can you elaborate why "85" is used here?

@amotl amotl requested a review from maurerle December 3, 2024 23:51
@amotl amotl marked this pull request as ready for review December 3, 2024 23:51
@intermittentnrg
Copy link
Collaborator Author

85 was the height of the firefox chrome (location bar etc) and was being zoomed together with the page using this method if I recall correctly.

@amotl
Copy link
Contributor

amotl commented Dec 4, 2024

I see. Thanks for clarifying. I will add this information per inline comment on the relevant spot.
So, let's merge and include the improvement into the next release, if you agree with the current shape of this patch?

@amotl amotl merged commit 378233b into grafana-toolbox:main Dec 4, 2024
5 checks passed
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.

3 participants