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

Schedules: Add timescale and time indicator #292

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

sukhwinder33445
Copy link
Contributor

@sukhwinder33445 sukhwinder33445 commented Feb 6, 2025

resolves #283

@sukhwinder33445 sukhwinder33445 self-assigned this Feb 6, 2025
@cla-bot cla-bot bot added the cla/signed CLA is signed by all contributors of a PR label Feb 6, 2025
@sukhwinder33445 sukhwinder33445 force-pushed the add-timescale-and-indicator branch 4 times, most recently from 62d68b8 to 505ac04 Compare February 11, 2025 13:08
@sukhwinder33445 sukhwinder33445 force-pushed the add-timescale-and-indicator branch from 505ac04 to 8ea7052 Compare February 12, 2025 09:35
Copy link
Member

@ncosta-ic ncosta-ic left a comment

Choose a reason for hiding this comment

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

Additional note: Your PR introduces a vertical scrollbar. This is probably caused by your translate rules. Please recheck your code and fix it.

EDIT: A overflow: hidden on line 67 in timeline.less fixes it

use ipl\Web\Style;
use Locale;

class Timescale extends BaseHtmlElement
Copy link
Member

Choose a reason for hiding this comment

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

The class documentation is missing (PHPDoc)

Comment on lines 84 to 92
$allTimestamps = array_merge(...array_fill(0, $this->days, $dayTimestamps));

$lastPosition = count($allTimestamps) - 1;

$fakeLast = clone $allTimestamps[$lastPosition];
$allTimestamps[$lastPosition] = $fakeLast->addAttributes(['class' => 'last']);
// added the class to add them into the same cell
$first = clone $allTimestamps[0];
$allTimestamps[] = $first->addAttributes(['class' => 'midnight']);
Copy link
Member

Choose a reason for hiding this comment

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

Maybe a description would be helpful as to why the clones are needed (array_fill works with references and fills the array positions with references to the same $dayTimestamps array, changing something on its contents, changes it in all positions).

new HtmlElement(
'div',
new Attributes(['class' => 'now', 'title' => $dateFormatter->format($now)]),
Text::create('now')
Copy link
Member

Choose a reason for hiding this comment

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

This is missing a translation

@ncosta-ic
Copy link
Member

ncosta-ic commented Feb 28, 2025

@sukhwinder33445 I've also noticed that something's off in Firefox. While the timeline and grid render correctly, the timescale's off. It's especially noticeable when using the monthly view.
I can't test it on Safari, but you might want to double-check that one as well.
difference_chrome_firefox

@sukhwinder33445 sukhwinder33445 force-pushed the add-timescale-and-indicator branch 2 times, most recently from 0384a27 to a40eb0f Compare March 3, 2025 09:27
Copy link
Contributor

@flourish86 flourish86 left a comment

Choose a reason for hiding this comment

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

Looks fine so far.

There're one thing I noticed, though.

The cursor is inapporpriate in this case, since there's nothing happening on click.
Frame 110

The scale labels still look cluttered especially in month and 2 weeks mode. But we should postpone the refinement to another time.

@sukhwinder33445 sukhwinder33445 force-pushed the add-timescale-and-indicator branch from a40eb0f to fead6e6 Compare March 4, 2025 10:49
@sukhwinder33445
Copy link
Contributor Author

sukhwinder33445 commented Mar 4, 2025

Firefox still has problems (@flourish86 and @nilmerg say it's not that important), but Safari and Chrome work perfectly now.

background-color: @gray-light;
font-size: 0.75em;
color: red;
user-select: none;
Copy link
Contributor

Choose a reason for hiding this comment

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

I found, that it won't work on Safari and a Quick Look here shows, that you probably need the prefixes for all browsers to make this work.

So I'd suggest to add

-webkit-user-select: none;
-moz-user-select: none;

and optionally

-ms-user-select: none;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahmm, fortunately we have a mixin for that.

Copy link
Contributor Author

@sukhwinder33445 sukhwinder33445 Mar 4, 2025

Choose a reason for hiding this comment

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

Safari disables the selection, but still shows the caret cursor 🤦.

@sukhwinder33445 sukhwinder33445 force-pushed the add-timescale-and-indicator branch from 9f54900 to 5d70ce3 Compare March 18, 2025 09:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla/signed CLA is signed by all contributors of a PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Clarify temporal direction of the timeline
3 participants