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

Add support for /time, dynamic start and end times #19

Closed
wants to merge 2 commits into from
Closed

Add support for /time, dynamic start and end times #19

wants to merge 2 commits into from

Conversation

Conceptron
Copy link

  • The start and end times are now calculated as the min / max of the start and end times of the events. This improves the use of space in the visualization
  • Events can now use "x" as end times to represent current time

- The start and end times are now calculated as the min / max of the start and end times of the events. This improves the use of space in the visualization
- Events can now use "x" as end times to represent current time
Copy link
Owner

@hannesfrank hannesfrank left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution @Conceptron!

I like making the start and end time more dynamic and flexible! Although it

  1. removes some robustness, e.g. the user can enter 9999 which is like 4 days and this makes it harder to tell what's going on. Maybe it's cool to be able to plan two consecutive days in a single diagram though!
  2. It's harder to visualize your day, e.g. if you have configured the start and end time matching your working hours you have an empty block to fill. With purely dynamic start and end times the shown interval always changes.

I have not run the code myself yet, but it looks like start and end time are undefined/NaN when there are no time blocks yet?

Maybe we can still use DEFAULT_START_TIME and DEFAULT_END_TIME when (they are explicitly defined by the user AND the defined time blocks do not go out of this interval) OR there are no blocks yet (even if the user has not defined anything, then use 600, 2200 or similar). I think this would require changing the plugin settings mechanism.

I like the support for x in end times and ignoring : in time definitions!

I'll approve as I feel my concerns are overcomplicating things and I like the general changes!

Comment on lines 197 to 200
} else if (block.end === 'x') {
// get current time in hhmm format
const now = new Date();
block.end = now.getHours() * 100 + (now.getMinutes() / 60) * 100;
Copy link
Owner

Choose a reason for hiding this comment

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

What happens if the start time of this block is in the future? Then this block would be reversed.

What happens if there are multiple blocks ending in x. I think this is fine? It would be great if we had #12 then.

This pull request was closed.
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.

2 participants