-
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. |
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 |
@maha-sachin Would you be able to make the minor changes as suggested and update the PR? Thanks! |
Sure, I'd make the suggested changes and update the PR. I'll start working on it and will notify you once it's updated. Thank you!
Sure, I'd make the suggested changes and update the PR. I'll start working on it and will notify you once it's updated. Thank you! |
Hi @sabine @shakthimaan , I hope you're doing well. I’ve been working on integrating a new JavaScript file named event_script.js for handling event-related functionality across multiple templates. In the table of contents component (components/toc.eml), we include the script as an OCaml value and render it on every page that has a table of contents. I wanted to know if this would be an appropriate approach. To be honest, I felt a bit unsure about the project structure and the best way to organize and include this script. Could you possibly help me fix this and guide me on the preferred structure? |
For a new feature like this, which is unlikely to be reused between different components, it seems best to co-locate the script with the component that is using the script. So I would put it all into
Then, anywhere the component is used, we have to insert |
…nd time, based on their presence
90ba962
to
d70e888
Compare
Hi @sabine & @shakthimaan , I wanted to inform you that I have implemented the requested fix for the issue discussed in PR #2339 on the ocaml/ocaml.org repository. You can find the specific commit ((91e251e)) Could you please review the changes at your earliest convenience? Your feedback would be greatly appreciated to ensure everything is in order. Thank you for your time and assistance. Best regards, |
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!