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

UI: apply timezone settings #2124

Merged
merged 2 commits into from
Jun 21, 2023
Merged

Conversation

ysknkd
Copy link
Contributor

@ysknkd ysknkd commented Apr 6, 2023

Background

At present, the time displayed in the UI is set to UTC by default. Consequently, users of the UI are required to individually convert the displayed time to their respective local time zones, which can be rather inconvenient.

Overview

In this pull request, I intend to modify the UI so that the displayed time aligns with the server's TimeZone setting, thereby enhancing user experience.

To achieve this, the React components have been updated to obtain TimeZone settings from Redux. I was uncertain about the most appropriate Redux Store for storing the TimeZone settings and ultimately decided on the domains store. If you believe there is a more suitable location for these settings, kindly share your insights.

Additionally, as the TimeZone will be utilized across the majority of pages within the UI, I have incorporated the necessary changes into _app.js. If there is a more efficient or effective approach to implementing this feature, please do not hesitate to share your suggestions.

Signed-off-by: Yosuke Nakada <[email protected]>
@ysknkd
Copy link
Contributor Author

ysknkd commented May 30, 2023

I apologize for the large commit. If using redux is not appropriate, I'm thinking of exploring other methods. I would appreciate any comments you may have.

@abvaidya
Copy link
Collaborator

Apologies for the delay. Our team has started looking into it but they haven't completed the review yet. I will check with them this week. Thank you for your patience :)

}
let review = this.props.item.reviewReminder;
if (review) {
review = this.localDate.getLocalDate(review, 'UTC', 'UTC');
review = this.localDate.getLocalDate(exp, this.props.timeZone, this.props.timeZone);
Copy link
Contributor

Choose a reason for hiding this comment

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

should be review instead of exp

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mendi160 Thank you for your confirmation. I've fixed it.

Signed-off-by: Yosuke Nakada <[email protected]>
Copy link
Contributor

@mendi160 mendi160 left a comment

Choose a reason for hiding this comment

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

Screenshot 2023-06-02 at 12 20 14 My time zone is IDT (Israel) and I get a different time zone. What I don't understand is why I don't get the same time zone for the second member's expiration and the second member's review reminder

@havetisyan
Copy link
Collaborator

Screenshot 2023-06-02 at 12 20 14 My time zone is IDT (Israel) and I get a different time zone. What I don't understand is why I don't get the same time zone for the second member's expiration and the second member's review reminder

@mendi160 that's actually correct and is not a different time zone. Both are Pacific Time Zone - however, based on the date, it correctly reports the daylight savings time in effect on that date:
PDT - Pacific Daylight Time zone
PST - Pacific Standard Time zone

Copy link
Contributor

@mendi160 mendi160 left a comment

Choose a reason for hiding this comment

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

@ysknkd I couldn't understand how it works. I saw that in the end, you take the timezone from the config file, but how it is set with the user time zone? please point me to what I'm missing, thanks

@ysknkd
Copy link
Contributor Author

ysknkd commented Jun 6, 2023

@mendi160 I apologize for the confusion, my explanation was not clear. This PR does not change the display according to each user's timezone. In our environment, we basically operate in JST and our users are also familiar with JST. Therefore, we wanted to convert the timezone displayed in the UI to JST, which led to this change.

@mendi160
Copy link
Contributor

mendi160 commented Jun 6, 2023

@mendi160 I apologize for the confusion, my explanation was not clear. This PR does not change the display according to each user's timezone. In our environment, we basically operate in JST and our users are also familiar with JST. Therefore, we wanted to convert the timezone displayed in the UI to JST, which led to this change.

If so, it LGTM

@abvaidya abvaidya merged commit d4ede1d into AthenZ:master Jun 21, 2023
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.

4 participants