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

feat: add grid lines #1179

Open
wants to merge 16 commits into
base: dev
Choose a base branch
from
Open

Conversation

JulienDeveaux
Copy link

Implements #837

image
This is a grid_line_type of day for an hours_to_show of 730

README.md Outdated Show resolved Hide resolved
src/main.js Outdated Show resolved Hide resolved
src/main.js Outdated Show resolved Hide resolved
src/main.js Outdated Show resolved Hide resolved
src/main.js Outdated Show resolved Hide resolved
src/main.js Outdated Show resolved Hide resolved
src/main.js Outdated Show resolved Hide resolved
src/main.js Outdated Show resolved Hide resolved
src/main.js Outdated Show resolved Hide resolved
src/main.js Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
src/main.js Outdated Show resolved Hide resolved
src/main.js Outdated Show resolved Hide resolved
src/main.js Outdated Show resolved Hide resolved
src/main.js Outdated Show resolved Hide resolved
src/main.js Outdated Show resolved Hide resolved
src/main.js Outdated Show resolved Hide resolved
src/main.js Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
src/main.js Outdated Show resolved Hide resolved
@ildar170975

This comment was marked as outdated.

README.md Outdated Show resolved Hide resolved
src/main.js Outdated Show resolved Hide resolved
@ildar170975

This comment was marked as outdated.

@ildar170975
Copy link
Collaborator

@akloeckner
Please have a look at this PR.

Guys, I am away from PC, hope will be back in a week. Meanwhile please test this PR.

@@ -168,6 +167,8 @@ All properties are optional.
| labels_secondary | `hover` | `true` / `false` / `hover` | Display secondary Y-axis labels.
| name_adaptive_color | `false` | `true` / `false` | Make the name color adapt with the primary entity color.
| icon_adaptive_color | `false` | `true` / `false` | Make the icon color adapt with the primary entity color.
| grid_lines_type | `hour` | `5minute` / `hour` / `day` / `week` | Show grid lines dependently on `hours_to_show` value.
Copy link
Collaborator

@ildar170975 ildar170975 Dec 20, 2024

Choose a reason for hiding this comment

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

Let's change it to:

grid_lines_type | hour | false / 5minute / hour / day / week | Show grid lines dependently on hours_to_show value (set to false or omit this option if grid lines should not be displayed).

I hope this code:

const grid_lines_type = show.grid_lines_type ? show.grid_lines_type : false;

does process the show.grid_lines_type = false case properly.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It should. However, it could be simplified. ;-)

Also, I think, we should consider to have an option of grid_lines_per_hour, rather than grid_lines_type. It would allow for more flexibility with much simpler code. But I haven't thought this to the end yet.

Copy link
Collaborator

Choose a reason for hiding this comment

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

grid_lines_per_hour, rather than grid_lines_type

The initial idea was:
-- "main" (thick) lines are shown every 5 minutes / hourly / daily / ... ;
-- "secondary" (thin) lines are shown more frequently ("ratio" option) or not shown at all.
I honestly cannot see how grid_lines_per_hour may be used along with some "show secondary lines" option (or I am still far away from HA & normal life, need more time)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Just from the top of my head, I was thinking:

  • Gridlines per hour is basically the same as proposed now. We could simply use fractions to yield days or weeks. But we'd be able to also have a gridline each second if need be.
  • (It would also be consistent with the default points_per_hour setting. And reduce repeated code to actually convert days or weeks into multiples of hours.)
  • We could also omit the secondary grid lines as different "class". And use nth-of-type in css instead. Again it reduces code and is more customizable by the user.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Please let me think a bit.

Copy link
Collaborator

@ildar170975 ildar170975 Dec 26, 2024

Choose a reason for hiding this comment

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

@akloeckner
Was thinking about using grid_lines_per_hour:

  1. Currently we got new proposed options - grid_lines_type (false / 5minute / hour / day / week) & grid_lines_ratio (integer, >=0).
    Alternatively, we could get smth like grid_lines_per_hour (for thick lines) and same grid_lines_ratio (for thin lines).
    And in this case this grid_lines_per_hour is numerical (could be fractional < 1).
    Let's consider the "<1" case - it is needed to show thick lines, for example, every 2 hours, every day etc.
    For the "every day" case a user will have to use a 0.041667value.
    Does not look nice, a 1/24 value seems to be more user-friendly - but in this case we need to process these 1/x values and convert them to valid floats.

  2. If we choose the current implementation with grid_lines_type - we will have a minimal granularity for thick lines = 5 minutes.
    A smaller granularity then could be achieved by grid_lines_ratio:
    5 - thin lines every 1 minute,
    10 - every 30 seconds, and so on.
    Probably this would be sufficient for most scenarios.

We could also omit the secondary grid lines as different "class". And use nth-of-type in css instead. Again it reduces code and is more customizable by the user.

Sorry, I think using nth-of-type will make thing more difficult.
Assume we got a set of thick & thin lines WITHOUT classes.
To style them, we could use nth-of-type or similar - but a formula could be complex.
We draw these lines from a right side (now -> past). So, we will have to process these nth from the end as well. Not easy I think.
Using a separate class for thick & thin lines will allow users to quickly show/hide them by card-mod.

src/main.js Outdated Show resolved Hide resolved
src/main.js Outdated Show resolved Hide resolved
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.

3 participants