Closed kum-deepak closed 2 years ago
Once the sample is finalized, I will update the comments which are used by docco.
I have moved back most of the ColorMixin helpers from compact to the mainline version. Now the code reads much better.
I looked at the bubble chart color scale before and after, and I agree this is a big improvement. Could we call it
colorScale
instead ofscaledColors
? As "color scale" is a standard D3 concept name.Good idea, pushed as a commit.
In general I think we should try to keep the external interface as simple as it was, while cleaning up internal interfaces. This means that there will usually have to be sugar like this to hide the Helpers. Ideally users would only encounter Helpers if they are trying to do something extraordinary. (Example: if we didn't already have a precedent for
colorCalculator
, this would be the kind of deep customization where using the Helpers makes sense.)Makes perfect sense. During updating other examples, I am sure more such cases would surface.
I have a general question about
.configure()
; apologies if we have discussed this before. Which (kinds of) properties now go into.configure()
, and which ones remain as getter/setters with private fields? Example: I see thatminWidth
is now a configuration property, butwidth
andheight
remain as fields. What is the rule of thumb for which goes where? Is the goal for all properties eventually to go in the configuration?In this example, could
colorScale
be a configuration property?In general I like
.configure()
better, because the order of setting properties should not matter. (As we know, it currently does matter in a lot of places.)I'd like to see the "Key/value pair configuration" rule of thumb / rationale spelled out in the migration guide, as well as the DataProvider concept. All this stuff should become clear as we do these reviews.
Currently, for charts, I have kept the following criteria:
Any attribute that is never updated by the chart itself. My thinking was to guarantee that if any attribute is set and queried again, it must return the same value. For example, width
is changed inside the chart, so, it has been kept out. Another example, gap
in a bar chart gets updated, so, it is not included. I am not too passionate about this criterion, so, we may decide to remove that. (This criterion also helps in ensuring that order does not matter).
The conf must be shareable across multiple charts. This gives an important criterion that all values must be immutable. This means objects like scales should not be moved to the configuration system.
In general, I am good with merging this, but let's continue to discuss the design rationale so that we can document it.
Following are the changes:
In general, the newer syntax looks alright. The color scale setting in the case of Bubble Chart seems quite clumsy, I guess I will add suitable methods to the color mixin.
Based on your feedback, I will update other samples.