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

Fix to render panels and follow the selected timezone from URL query if specified. #224

Merged
merged 5 commits into from
Jun 10, 2021

Conversation

Bujupah
Copy link
Contributor

@Bujupah Bujupah commented May 12, 2021

Related to issue #223

Fixing the rendered panel timezone with a custom TZ from URL query.

Need someone to produce that and check if timezone is failing too, if so review this PR and check it too. this has fixed my issue.

Fixing the rendered panel timezone with a custom TZ from URL queries
@marefr marefr requested a review from AgnesToulet May 24, 2021 16:24
@AgnesToulet
Copy link
Contributor

Thanks for this PR! I did some tests and ran into the same issue. This PR fixes it but I have a few comments:

  • Setting the env variable in the LaunchOptions seems to not have any effect so it may be best to remove the code from the getLauncherOptions and move it into a separate function like this:
  async setTimezone(page, options) {
    const timezone = options.timezone || this.config.timezone;
    if (timezone) {
      await page.emulateTimezone(timezone);
    }
  }

@marefr WDYT? Do you know if it used to work?

  • It also needs to be added in the new renderCSV function to be complete.

@Bujupah
Copy link
Contributor Author

Bujupah commented May 27, 2021

yes! It kinda needs some refactoring tbh

@marefr
Copy link
Member

marefr commented May 27, 2021

Setting the env variable in the LaunchOptions seems to not have any effect so it may be best to remove the code from the getLauncherOptions and move it into a separate function like this:

I'm pretty sure it did work back in the days. But if this works all is good 👍

@Bujupah
Copy link
Contributor Author

Bujupah commented May 28, 2021

Thanks for this PR! I did some tests and ran into the same issue. This PR fixes it but I have a few comments:

  • Setting the env variable in the LaunchOptions seems to not have any effect so it may be best to remove the code from the getLauncherOptions and move it into a separate function like this:
  async setTimezone(page, options) {
    const timezone = options.timezone || this.config.timezone;
    if (timezone) {
      await page.emulateTimezone(timezone);
    }
  }

@marefr WDYT? Do you know if it used to work?

  • It also needs to be added in the new renderCSV function to be complete.

I updated my PR with this function

@AgnesToulet
Copy link
Contributor

Sorry for the delay, thanks for the update! Could you also add it to the renderCSV function please to have the same behavior?

And also, could you merge with master to fix the issues with the pipeline? Thanks!

update the browser timezone in csv rendering
@Bujupah
Copy link
Contributor Author

Bujupah commented Jun 9, 2021

No worries @AgnesToulet and thanks.
I made that change

@AgnesToulet AgnesToulet merged commit 378d82e into grafana:master Jun 10, 2021
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