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

maxProperty doesn't have a good way to use "default" #77

Open
jamesarosen opened this issue Jul 2, 2015 · 17 comments
Open

maxProperty doesn't have a good way to use "default" #77

jamesarosen opened this issue Jul 2, 2015 · 17 comments

Comments

@jamesarosen
Copy link
Contributor

😄 If I don't pass a yMax, I get a nice default behavior:

{{ember-nf-graph}}

😄 If I do pass a yMax, I can get a nice override:

{{ember-nf-graph yMax=100 class='percent-graph'}}

😢 But there's no way for me to use yMax=someBoundProperty that might be undefined or might be an integer. If it's undefined, then yMax gets set to undefined, not to the default behavior.

I think an easy solution would be to change the logic in the maxProperty to use the override only if it's not null:

// was:
if(arguments.length > 1) {
  this[__Max_] = value;
} else {

// suggested:
if (value != null) {
  this[__Max_] = value;
} else {
@benlesh
Copy link
Contributor

benlesh commented Jul 6, 2015

This is tricky. It seems like a few cases are not covered:

  • null
  • undefined
  • NaN
  • Infinity

I think the behavior you're suggesting is perfect for undefined, where you just don't change whatever it's been set to. But for null or NaN, I think it should probably go to 0 or some minimal number, because it seems like in those cases the consuming code is specifically getting at changing the value to something, even if it's not apparent what that something is.

Thoughts, @jayphelps?

@benlesh
Copy link
Contributor

benlesh commented Jul 6, 2015

.. and on the Infinity side, I think that's an extreme edge case, but I can probably default that to MAX_VALUE or MIN_VALUE depending on the polarity of the thing.

@jayphelps
Copy link
Member

@jamesarosen so I have a good understanding, what's a use case for binding the property, but the value is undefined/null but it is not a bug? I've found myself more and more not doing these sort of "I can't use the value, fall back to default" because they tend to mask bugs in apps that later are hard to track down. Instead, I usually prefer to either throw an actual error or fallback with a console.warn() that the value provided wasn't usable and it's falling back.

@jamesarosen
Copy link
Contributor Author

My use-case: I'm building a component that wraps nf-graph. It has a unit that could be "percent", "byte", or "count". If it's "percent", then yMax should be 100. Otherwise, it should be auto.

My implementation is

// my-graph.js
yMax: computed("unit", function() {
  return this.get("unit") === "percent" ? 100 : undefined;
})

Alternatively, I could pass in something like "auto" to indicate I want the default behavior.

@jayphelps
Copy link
Member

@jamesarosen that seems like a very valid case. I dig your suggestion for "auto" to let nf-graph know to use the default behavior. Back to you @Blesh.

@jamesarosen
Copy link
Contributor Author

In that case, the docs might read something like

yMax: maximum value on the y-axis. Defaults to "auto", which causes the axis to use a value slightly higher than the highest value in the data.

And my example might look something like

{{nf-graph yMax=(if isPercent 100 "auto")}}

@benlesh
Copy link
Contributor

benlesh commented Jul 6, 2015

It's tricky. I sort of view the value the graph was initially set with to be the "default", not necessarily the coded default. It seems like we could memoize that when it's initialized, somehow. I think passing undefined and expecting it to default is fine. I'm more concerned with that matching up with where someone is inadvertently passing in NaN due to some miscalculation, I guess.

Perhaps it should be like this:

{{#nf-graph xMax=someValue xMaxDefault=100}}

Where if someValue is undefined or null it gets set to xMaxDefault...

However, in the event that someValue is NaN it should be set to 0 and MIN_VALUE or MAX_VALUE for ±Infinity.

Thoughts?

@jamesarosen
Copy link
Contributor Author

What's the current behavior if I set yMax=NaN?

@benlesh
Copy link
Contributor

benlesh commented Aug 26, 2015

It should behave the same as a 0, I think.

@jamesarosen
Copy link
Contributor Author

So you want

  • undefined, null, NaN, 0 to all be treated as "calculate max"
  • any number to be treated as a fixed value

@benlesh
Copy link
Contributor

benlesh commented Sep 14, 2015

I really depends on the yMaxMode how it will behave. If it's auto it should ignore setting the value entirely.

@jamesarosen
Copy link
Contributor Author

@Blesh can you give me very specific requirements? I need to work on this this week to ship a feature. I can make up an implementation that works for me, but I'd rather do something that will get merged here (eventually).

@benlesh
Copy link
Contributor

benlesh commented Sep 14, 2015

different yMaxModes:

  • auto: automatically calculate yMax from contained graphics' data
  • push: allow setting of yMax, but push the yMax value up if the contained data passes it.
  • push-tick: same as push, but go to the next nicest "tick" value.
  • fixed: user determines the yMax value at all times.

When in doubt, use fixed and set it yourself. We do this for things like sliding windows, etc.

Does that help?

@jamesarosen
Copy link
Contributor Author

These yMaxModes suggest maybe I'm going about this wrong. Imagine the following use-case:

When showing a percentage graph, default to showing [0-100%] by default. Allow the user to zoom in on the displayed range.

Given that, I would guess something like

{{nf-graph yMax=100
           yMin=0
           yMaxMode=(if isZoomed "push" "fixed")
           yMinMode=(if isZoomed "auto" "fixed")}}

@jamesarosen
Copy link
Contributor Author

I'm totally cool with setting yMax to "auto" or some other string when I want default behavior:

{{nf-graph yMax=(if isZoomed "auto" 100)}}

@benlesh
Copy link
Contributor

benlesh commented Sep 17, 2015

The method that is going to give you the most control will be using fixed in most cases. auto and push or push-tick were really added as conveniences, but might not be doing the most efficient thing when trying to suss out where your min and max values lie.

@benlesh
Copy link
Contributor

benlesh commented Sep 17, 2015

but might not be doing the most efficient thing when trying to suss out where your min and max values lie.

For example, maybe your array is already sorted, or maybe your service already gave you the min and max values, and you can just pick them out of an object or an array without having to iterate over that array.

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

No branches or pull requests

3 participants