Accessors #601

mcnuttandrew · 2017-09-08T21:40:37Z

This sprawling PR adds support for accessors throughout the library, thus addressing #360. It modifies the way that scales are built, by replacing a default notion of attr with a comprable notation of accessor, while adding default accessors that emulate the original behaviour. (Hopefully) This change is non-breaking, in that all old charts should still work, with the new accessor functionality enabled.

Note: one of the original goals of adding accessors was to enable immutable manipulations, and while were close, there is probably one or two more PRS worth of clean up around the library that needs to be done first. (i'm looking at you, several for loops still left in the library)

There still needs some documentation, but should be ready to review.

ibgreen

Just as a point of reference:

In deck.gl accessors are consistently called get...: getSize, getAngle etc. (although getX sounds a little strange?)

And they always have "natural" defaults:

getX: obj => obj.x,
getAngle: obj => obj.angle

mcnuttandrew · 2017-09-08T21:55:12Z

In deck.gl accessors are consistently called get...: getSize, getAngle etc. (although getX sounds a little strange?)

I guess react-vis has a slightly different API pattern? Generally when specifying a property about a dimension (eg the type of scale to be used or the domain) we specify that as xType or xDomain. I'm not opposed to copying deck.gl's api style, I just felt like the right move was to be consistant within the library first, and change later?

ibgreen · 2017-09-08T22:02:07Z

the right move was to be consistant within the library first, and change later?

That does make sense. Although given today's conversation I think we'd want to decide and possibly change before we publish an official version.

I am not sure how the branching strategy in react-vis works, how long master can live without being published etc. If we land this, are we creating an urgency to decide and update?

mcnuttandrew · 2017-09-08T22:09:31Z

I am not sure how the branching strategy in react-vis works, how long master can live without being published etc. If we land this, are we creating an urgency to decide and update?

Generally our strategy has been to develop on master, however if we want this to be part of some sort v2 release, we could hold all new pieces on a separate branch. I'm in no huge rush to get this is, so whatever is fine.

mcnuttandrew · 2017-10-05T14:52:39Z

Bump for review?

ibgreen · 2017-10-05T15:00:49Z

@mcnuttandrew Based on conversations with @jckr I came away with the sense that we want accessor names to be aligned with the suite (i.e. deck.gl) from the start, rather than introduce one set of names one month and then introduce a completely different set next month. At the same time, the deck.gl alignment might (at least initially) only cover a small part of the charts.

mcnuttandrew · 2017-10-05T15:28:23Z

While I appreciate that it is good to have things consistent across the suite, I think it is, perhaps, more important to have them be consistent within each library. The react-vis pattern is xType, xDomain, etc, so it seems like it makes sense to continue that into xAccessor? I guess if you really wanted to be consistent across the suite, react-vis would need to move to a naming convention like getXType getXDomain etc? But that seems kinda awkward to me?

I don't have a strong opinion, I'd mostly just like to get this merged in so that it doesn't to be defended against merge conflicts.

ibgreen · 2017-10-05T15:54:52Z

I guess if you really wanted to be consistent across the suite, react-vis would need to move to a naming convention like getXType getXDomain etc? But that seems kinda awkward to me?

Yes that is what is being proposed. In effect, we are trying to understand whether react-vis should be part of the suite or live alone. If part of the suite, there needs to be a story and a plan that gradually ties things closer together. No decisions made, just discussions at this point. I don't have specific goal in mind, I am mostly getting involved because I think react-vis deserves a strong vision and roadmap, and suite alignment could provide that.

jckr · 2017-10-05T16:13:04Z

Yeah that's the plan. if we do add accessors which your PR allows us to do then it's a great opportunity to move towards suite alignment and to revise the whole syntax of the API.

mrawdon · 2017-10-19T14:10:36Z

From a user standpoint this PR(in what ever form) would be a big help, and save a lot of extra data manipulation.

ibgreen · 2017-10-19T14:33:40Z

From a user standpoint this PR(in what ever form) would be a big help, and save a lot of extra data manipulation.

if we do add accessors which your PR allows us to do then it's a great opportunity to move towards suite alignment and to revise the whole syntax of the API.

I took a second look. My take is that if we just update this PR to follow the getX instead of xAccessor naming pattern, this would be a major move towards suite alignment and we could do additional syntax revision in a second step.

We could get the accessors into react-vis today in a backwards compatible minor version with minimal effort, score a big alignment win, and save a bigger API overhaul for a later major version.

That said, if I was reviewing another framework in our suite, I would ask that the component doc pages and a "What's New" doc page to be updated in this PR :)

mcnuttandrew · 2017-10-19T14:36:26Z

@ibgreen Those are reasonable notes! I can take a shot at getting this wrapped up this weekend.

mcnuttandrew · 2017-11-19T18:07:52Z

Alright, I have moved all of the accessors over to the getX pattern. Currently this PR is non-breaking, but so it can be slid in without a fuss. I didn't update the documentation in a more sprawling way, because I figured we'd only want to do that once every property had been moved to the getXType pattern. That said, I still want to argue against using getXType: getX makes sense you are providing a function to get something, while getXType does not, XType is a static string that won't change over time.

There's still a couple of things still to do, like moving nullAccessor to be getNull, and I've open an issue to that effect (move nullAccessor to be getNull #661).
Following @ibgreen's suggestion i've also opened an issue to make a what's new page (Make a what's new page #662)

mcnuttandrew · 2017-11-21T22:46:19Z

bump for review (@balthazar or @ibgreen or ...?)

ibgreen

The accessor0 naming pattern looks rather unnatural to me. I've added a little more context in the comments. If the result of careful deliberations, then fine, otherwise happy to bounce ideas for alternatives.

Other than that, looks good. Merge at will.

ibgreen · 2017-11-29T16:55:55Z

src/utils/scales-utils.js

@@ -177,19 +186,19 @@ export function getScaleFnFromScaleObject(scaleObject) {
 /**
 * Get the domain from the array of data.
 * @param {Array} allData All data.
- * @param {string} attr Property name.
+ * @param {function} accessor - accessor for main value.
+ * @param {function} accessor0 - accessor for the naught value.


Something about these names bother me, especially accessor0. Maybe call the params, mainAccessor, zeroAccessor

Haven't read carefully, but why do we have an accessor for zero as opposed to just supplying a value? Assuming this is a single value?

+1 for dropping attr, by the way.

So to clarify you are suggesting a naught accessor like zeroX and zeroAngle?

To to clarify you are suggesting a naught accessor like zeroX and zeroAngle?

To be honest, I am not completely sure what I am proposing as I don't fully understand what the zero accessors are doing. Mainly reacting to the name which looks unusual and not immediately obvious to a new comer.

First of all, I am asking if accessor0 needs to be an accessor, e.g.

Is accessor0 called on every item, then yes,

but if accessor0 is called just once, to get a single "zero" value as reference point for the series, could we just supply the zero value directly rather than ask for another accessor?

If only once, then yes, (value) props could be called zeroX and zeroAngle

If it needs to be an accessor then my case is weaker. It could still be worth at least looking at naming options, like getZeroX and getZeroAngle.

The zero accessor tends to come up in a few particular types of cases. For instance if you have rectangle expressed through RectSeries, you'd express the bottom of the rectangle through getY0 and the top through getY. Similarly, if you were working with an ArcSeries, getAngle0 would express the start angle of the wedge and getAngle would express the end angle. Each of these refers to values found on the data eg {angle0: Pi, angle: Pi / 2} or {y0: 2, y: 4}, so it'd be a pretty big change to alter this naming convention too.

So, yes getAccessor0 needs to be called on every datum for some series. It doesn't really refer to a zero persay, so much as a naught: a secondary variable specifying the beginning of something.

Got it. Starting fresh, I would personally favor changing the expected data fields to startAngle etc. But agree that staying consistent now makes the most sense.

Perhaps something to consider in the API audit @jckr is planning.

ibgreen · 2017-11-29T16:58:11Z

src/sunburst/index.js

@@ -175,20 +193,29 @@ class Sunburst extends React.Component {
 Sunburst.displayName = 'Sunburst';
 Sunburst.propTypes = {
  animation: AnimationPropType,
+  getAngle: PropTypes.func,
+  getAngle0: PropTypes.func,


See my comment below about names ending with 0.

ibgreen · 2017-11-29T16:58:34Z

src/sunburst/index.js

+function buildLabels(mappedData, accessors) {
+  const {
+    getAngle,
+    getAngle0,


See comment below about names ending with 0.

* Accessors * rebase and fix tests * shift from xAccessor to getX pattern * forgot that this pr introduced accessors on parallel coords oto

mcnuttandrew requested review from balthazar and jckr September 8, 2017 21:40

ericsoco requested review from Pessimistress and ibgreen September 8, 2017 21:45

ibgreen requested review from Pessimistress and removed request for Pessimistress September 8, 2017 21:45

ibgreen reviewed Sep 8, 2017

View reviewed changes

balthazar added the enhancement label Sep 15, 2017

mcnuttandrew force-pushed the accessors branch from 256d74b to f9fd5d3 Compare September 22, 2017 00:50

mcnuttandrew force-pushed the accessors branch from f9fd5d3 to a944325 Compare November 19, 2017 17:55

ibgreen approved these changes Nov 29, 2017

View reviewed changes

mcnuttandrew added 4 commits November 29, 2017 22:32

Accessors

d7448ee

rebase and fix tests

935bfb2

shift from xAccessor to getX pattern

8356df7

forgot that this pr introduced accessors on parallel coords oto

32f0c0c

mcnuttandrew force-pushed the accessors branch from 234c509 to 32f0c0c Compare November 30, 2017 04:34

mcnuttandrew merged commit 8ac2534 into uber:master Nov 30, 2017

mcnuttandrew deleted the accessors branch September 23, 2018 16:13

ayarcohaila pushed a commit to ayarcohaila/react-vis that referenced this pull request Jun 30, 2021

Accessors (uber#601)

519e6ec

* Accessors * rebase and fix tests * shift from xAccessor to getX pattern * forgot that this pr introduced accessors on parallel coords oto

ayarcohaila added a commit to ayarcohaila/react-vis that referenced this pull request May 30, 2023

Accessors (uber#601)

ea0d29d

* Accessors * rebase and fix tests * shift from xAccessor to getX pattern * forgot that this pr introduced accessors on parallel coords oto

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Accessors #601

Accessors #601

mcnuttandrew commented Sep 8, 2017

ibgreen left a comment

mcnuttandrew commented Sep 8, 2017

ibgreen commented Sep 8, 2017 •

edited

mcnuttandrew commented Sep 8, 2017

mcnuttandrew commented Oct 5, 2017

ibgreen commented Oct 5, 2017

mcnuttandrew commented Oct 5, 2017

ibgreen commented Oct 5, 2017

jckr commented Oct 5, 2017

mrawdon commented Oct 19, 2017

ibgreen commented Oct 19, 2017

mcnuttandrew commented Oct 19, 2017

mcnuttandrew commented Nov 19, 2017 •

edited

mcnuttandrew commented Nov 21, 2017

ibgreen left a comment

ibgreen Nov 29, 2017

mcnuttandrew Nov 29, 2017

ibgreen Nov 29, 2017

mcnuttandrew Nov 29, 2017 •

edited

ibgreen Nov 29, 2017

ibgreen Nov 29, 2017

ibgreen Nov 29, 2017

Accessors #601

Accessors #601

Conversation

mcnuttandrew commented Sep 8, 2017

ibgreen left a comment

Choose a reason for hiding this comment

mcnuttandrew commented Sep 8, 2017

ibgreen commented Sep 8, 2017 • edited

mcnuttandrew commented Sep 8, 2017

mcnuttandrew commented Oct 5, 2017

ibgreen commented Oct 5, 2017

mcnuttandrew commented Oct 5, 2017

ibgreen commented Oct 5, 2017

jckr commented Oct 5, 2017

mrawdon commented Oct 19, 2017

ibgreen commented Oct 19, 2017

mcnuttandrew commented Oct 19, 2017

mcnuttandrew commented Nov 19, 2017 • edited

mcnuttandrew commented Nov 21, 2017

ibgreen left a comment

Choose a reason for hiding this comment

ibgreen Nov 29, 2017

Choose a reason for hiding this comment

mcnuttandrew Nov 29, 2017

Choose a reason for hiding this comment

ibgreen Nov 29, 2017

Choose a reason for hiding this comment

mcnuttandrew Nov 29, 2017 • edited

Choose a reason for hiding this comment

ibgreen Nov 29, 2017

Choose a reason for hiding this comment

ibgreen Nov 29, 2017

Choose a reason for hiding this comment

ibgreen Nov 29, 2017

Choose a reason for hiding this comment

ibgreen commented Sep 8, 2017 •

edited

mcnuttandrew commented Nov 19, 2017 •

edited

mcnuttandrew Nov 29, 2017 •

edited