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

soft_bound config #330

Merged
merged 2 commits into from
Apr 15, 2020
Merged

soft_bound config #330

merged 2 commits into from
Apr 15, 2020

Conversation

Silencer2K
Copy link
Contributor

A new config parameter that allows to extend the boundaries when graph is out of range

src/main.js Outdated
Comment on lines 733 to 740
? !config.soft_bound
? config.lower_bound
: Math.min(config.lower_bound, ...this.primaryYaxisSeries.map(ele => ele.min))
: Math.min(...this.primaryYaxisSeries.map(ele => ele.min)) || this.bound[0],
config.upper_bound !== undefined
? config.upper_bound
? !config.soft_bound
? config.upper_bound
: Math.max(config.upper_bound, ...this.primaryYaxisSeries.map(ele => ele.max))
Copy link
Contributor

Choose a reason for hiding this comment

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

Few things:

  • double inline conditions are evil ;), please refactor
  • you have added this option for main axis please do the same for secondary (to have consistency), I think you don't need a separate setting for it and you can reuse this one
  • please update documentation

If I understand correctly you want to have a fixed bound but in case of value out of range we should render it correctly, righ?

@kalkih this seems to me like a bugfix - not a new feature, no? I'm trying to think about scenario where such hard bounds are useful but TBH nothing reasonable comes to my mind

Copy link
Owner

Choose a reason for hiding this comment

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

I think it could potentially be confusing for the user if the bound is specifically set but then also is dynamic, so I like the idea of having this as an option.
While I too can't find a use case where this isn't preferred, I'm sure there are, so we should keep that option available.

If it's documented well we could probably make this enabled by default though.

@kalkih kalkih added the feature request New feature or request label Apr 15, 2020
@kalkih
Copy link
Owner

kalkih commented Apr 15, 2020

Closes: #114

src/main.js Outdated
Comment on lines 745 to 754
config.lower_bound_secondary === undefined
? Math.min(...this.secondaryYaxisSeries.map(ele => ele.min)) || this.boundSecondary[0]
: config.soft_bounds_secondary
? Math.min(config.lower_bound_secondary, ...this.secondaryYaxisSeries.map(ele => ele.min))
: config.lower_bound_secondary,
config.upper_bound_secondary === undefined
? Math.max(...this.secondaryYaxisSeries.map(ele => ele.max)) || this.boundSecondary[1]
: config.soft_bounds_secondary
? Math.max(config.upper_bound_secondary, ...this.secondaryYaxisSeries.map(ele => ele.max))
: config.upper_bound_secondary,
Copy link
Contributor

@maxwroc maxwroc Apr 15, 2020

Choose a reason for hiding this comment

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

Sorry but I sustain my opinion: nested inline conditions make code not clear and hard to read. Please change it to regular if statements.

Another idea came up to my mind now: what about not adding new setting entry but allow values for bounds like: ~0. The ~ prefix would mean that this is not hard boundary . What do you think?

I would do it in the following way:

this.bound = [
    this.getBoudary("min", config.lower_bound, this.primaryYaxisSeries, this.bound[0]),
    this.getBoudary("max", config.upper_bound, this.primaryYaxisSeries, this.bound[1]),
];
getBoudary(type, configVal, series, defaultVal) {
    if (configVal === undefined) {
        // dynamic boundary depending on values
        return Math[type](...series.map(ele => ele[type])) || defaultVal
    }

    if (configVal[0] != "~") {
        // fixed boundary
        return configVal;
    }

    // soft boundary (respecting out of range values)
    configVal = parseInt(configVal.substr(1));
    Math[type](configVal, ...series.map(ele => ele[type]));
}

@kalkih what do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea. Now config looks much better: Screenshot

README.md Outdated
@@ -95,8 +95,10 @@ This card is available in [HACS](https://github.com/custom-components/hacs/issue
| align_state | string | `left` | v0.2.0 | Set the alignment of the current state, `left`, `right` or `center`.
| lower_bound | number | | v0.2.3 | Set a fixed lower bound for the graph Y-axis.
| upper_bound | number | | v0.2.3 | Set a fixed upper bound for the graph Y-axis.
| soft_bounds | boolean | `false` | | Allow to extend fixed bounds when graph is out of range.
Copy link
Contributor

Choose a reason for hiding this comment

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

As @kalkih suggested it might be true by default

Copy link
Contributor

@maxwroc maxwroc left a comment

Choose a reason for hiding this comment

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

Looks perfect for me now but please update the doc. Mention there the ~ option in value

@kalkih
Copy link
Owner

kalkih commented Apr 15, 2020

Looks good, good idea with the ~ prefix, great job you two.

@kalkih kalkih merged commit fc805c4 into kalkih:dev Apr 15, 2020
@ZeevoX
Copy link

ZeevoX commented Apr 28, 2020

It should probably be made clear in the README from which version onwards soft bounds are supported. As a new user I had no idea that soft bounds were newly introduced because it says from v0.2.3 and was wondering why they weren't working.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants