-
Notifications
You must be signed in to change notification settings - Fork 291
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: Added DateTime of Event on the Client Side in the User's Timezone #2339
base: main
Are you sure you want to change the base?
UI: Added DateTime of Event on the Client Side in the User's Timezone #2339
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @maha-sachin, thanks for doing this!
Some feedback:
- It will help to list the time zone to make it clear to people what time zone they're seeing this in
- only list hh:mm, not seconds.
- on an event with start and end time, the time zone should be only listed once
- on an event without start and end time, just the dates need to be shown
Hi @sabine , Thanks again for your valuable input! I'll make these adjustments to improve the readability and usability of the event details. |
Hi @sabine , Please review the changes and let me know if any further adjustments are needed. without start time |
Hey @maha-sachin, I pushed a patch to make this compile and added a check that events cannot have no start time, but an end time. For events with only a start time, currently, no time zone is being rendered: |
long term to fix the API right now in order to avoid multiplying the number of | ||
supported versions of the shape API in the various OCaml developer tools . | ||
|
||
We have thus released a second beta version of OCaml 5.2.0 to give the time to developer tools to update their 5.2.0 version ahead of the release (see below for the installation instructions). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have thus released a second beta version of OCaml 5.2.0 to give the time to developer tools to update their 5.2.0 version ahead of the release (see below for the installation instructions). | |
We have thus released a second beta version of OCaml 5.2.0, so there will be time for the developer tools to update their 5.2.0 version ahead of the release (see below for the installation instructions). |
I'm a little confused by this sentence. Does the above suggestion capture what you mean?
|
||
We have thus released a second beta version of OCaml 5.2.0 to give the time to developer tools to update their 5.2.0 version ahead of the release (see below for the installation instructions). | ||
|
||
Beyond this changes of API, the new beta contains three more bug fixes and three |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Beyond this changes of API, the new beta contains three more bug fixes and three | |
Beyond these API changes, the new beta contains three more bug fixes and three |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left a suggestion to hopefully clarify a sentence. If it's not accurate, we'll work together to make it more clear.
@christinerose these changes have nothing to do with this PR and are most likely included accidentally |
@sabine, I will update here once I fix the PR. |
…nd time, based on their presence
5d6a199
to
90ba962
Compare
Hi @sabine |
Hi @sabine, I have addressed the Timezone issue and reviewed the changes.
Could you please take some time to review it and provide your feedback? Your insights would be invaluable in ensuring that the changes meet the project's requirements and standards. Looking forward to hearing your thoughts! @sabine, Could you please help me understand what might be causing this ABOVE CI/Build error and how it can be resolved? Thank you for your assistance! |
Hi @christinerose @sabine , Could you please help me understand what might be causing this ABOVE CI/Build error and how it can be resolved? Thank you for your assistance! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @maha-sachin! This looks really nice, but there's two things I'd like to ask:
- can you turn the event card into a component, so that the same component is used in both the Events page and the Community page?
- can you move the JavaScript to a single place such that we don't have to repeat it in different templates (this could be done either by moving it into a file and including just the file in the template, or by having the script tag as a component).
ETA: you can ignore the build error because it is unrelated to your changes
@sabine , Sure thing! I'll work on turning the event card into a reusable component and consolidating the JavaScript into a single location to avoid repetition across templates. I'll make sure to get back to you once it's done. Thanks for pointing these out! |
@sabine & @christinerose , I've created a JavaScript file named event_script.js for handling event-related functionality across multiple templates. Could you please advise me on where would be the best place to store this file within our project folder structure? Additionally, when creating components in OCaml, I'd like to follow a consistent file naming convention. Could you let me know what the preferred naming convention is for component files in our project? Your guidance on these matters would be greatly appreciated. Thank you! |
In the table of contents component
We have a file |
Resolves #2327
Related Issue:: This PR addresses issue #2327 by adding the requested functionality to render DateTime in the user's timezone.
Description:
This PR adds functionality to render the DateTime of events on the client side in the user's timezone. Previously, the DateTime was displayed in UTC, which could be confusing for users in different timezones. With this update, the DateTime is dynamically converted to the user's local timezone using JavaScript's Date object.
Changes Made:
Modified the template to include JavaScript logic for converting DateTime to the user's timezone.
Updated the rendering of event start and end times to display in the user's local timezone.
Implemented handling for cases where no time is provided, displaying only the date.
Screenshots:
Before Modification:
After Modification:
Expected Output: This illustrates how the event date/time should be displayed in the user's local timezone.
@sabine , Could you please take some time to review it and provide your feedback? Your insights would be invaluable in ensuring that the changes meet the project's requirements and standards.
Looking forward to hearing your thoughts!