ihmeuw / ihme-ui Goto Github PK

Steps to reproduce:

1. Click on an area to select it
2. Move the mouse and click on an area to select it

Results:
The area will intermittently not toggle selected state

Notes:
This appears to be a problem in the onClick code.

Use props.disabled for that.

• classNameshould be applied here: https://github.com/ihmeuw/ihme-ui/blob/master/src/ui/axis/src/axis.jsx#L77

• axisClassNameshould be created and applied here: https://github.com/ihmeuw/ihme-ui/blob/master/src/ui/axis/src/axis.jsx#L51

• style should be applied here: https://github.com/ihmeuw/ihme-ui/blob/master/src/ui/axis/src/axis.jsx#L77

• axisStyleshould be applied here: https://github.com/ihmeuw/ihme-ui/blob/master/src/ui/axis/src/axis.jsx#L69

<MultiScatter presumably allows you to pass in props.symbolStyle, but that isn't passed through <Scatter /> to <Symbol />s.

Currently if
margins={{ top: ..., left: }}
is set in AxisChart, those margins are subtracted from given width and height via calcDimensions (see js:100), and then placed in dimensions (see js:84) for the state of the component.
When the svg is rendered, the width is computed by adding dimension.width + margins.left + margins.right (see js:113) making the svg width the same width that was passed originally. Similarly for height.
An actual svg margin would help visually to catch svg elements that flow over the svg border, as often happens with axis labels.

EDIT: So maybe my issue can be solved with inline styles involving padding. This may be a non-issue. I will investigate further.

When using very small numbers, floating point math errors can prevent the slider handles from being positioned properly. It may be better to calculate the list of possible values ahead of time and use a whole number scale to index into the list.

This would bring in functionality similar to the slider demo colors example.

Instead of propTypes.any: https://github.com/ihmeuw/ihme-ui/blob/master/src/ui/axis/src/axis.jsx#L117

Legend can benefit from being a pure component.

Each is listed as a CommonPropType.style, which allows both objects and functions. If a function is passed, it should be called with current datum or whole data array, as applicable.

...no reason why we can't expose a single <Select /> component that takes a multi prop.

warning.js:44Warning: Failed propType: Required prop 'scales' was not specified in 'MultiScatter'. Check the render method of 'App'.

warning.js:44Warning: Failed propType: Required prop 'scales' was not specified in 'Scatter'. Check the render method of 'App'.

MultiScatter and Scatter are passed scales from axis-chart. The console still logs these warnings.

Currently ChoroplethLegend makes assumption that color scale passed to it will be linear (hence inclusion of LinearGradient component), but it should support taking in color scale that is binned.

Currently, padding is subtracted from width and height within <AxisChart />. Those dimensions are then passed down to <AxisChart />'s children as width and height. Consider passing width and height without padding subtracted, and dealing with padding separately in each child component.

wrapperClassName -> className
wrapperStyles -> style

Will break mortality, gbd-search, potentially PSS

Steps to reproduce:

10 Press the "Toggle subnational" button.
20 GOTO 10

Results:

Eventually, all of the areas will turn red.

This component may be adding no value. Check for usefulness and remove if deemed useless.

Icon prop should have className and style applied.

Themes implementation has not received much attention. Maybe it should be removed entirely.

Currently, the only way to reliably provide the same fill to an entire <Scatter /> is to provide a dataAccessor.fill. While <MultiScatter /> exposes fieldAccessors.color, relying on it depends on const fillValue = propResolver(datum, dataAccessors.fill || dataAccessors.x); resolving to a non-finite value. That's wonky!

Providing fieldAccessor.color should reliably determine fill for an entire <Scatter />.

Unless we have a solid reason not to, we should allow specification of

• className: CommonProps.className (i.e., string, object, array)
• style: CommonProps.style (i.e., function or object)

on each component that renders a JSX intrinsic (e.g., div) as either a wrapper of other components or alone.

• Area
• Line
• MultiLine
• MultiScatter
• Scatter
• Symbol

It's a composition of XAxis and Scatter, along with some of its own, private components. Plus it really only exists because of IHME-specific viz requirements.

Currently limited to d3 scales. Add support for any scale function.

• Axis

Layer visibility activation has changed and layers.visible is out of date.

We should be working towards this library being tree-shakable. To support that effort, we should switch from the lodash library to the lodash-es library and update the import statements.

Padding defaults may not make sense, or may be implemented incorrectly. Make sure that padding object is complete and works properly.

Can we rename this to like, LoadingIndicator? Or something? It doesn't spin!

• onClick should not be required. Instead, a defaultProp of lodash/noop should be applied.

• style should be PropTypes.object as it does not support functions.

Desired API:

const data = dataGenerator({
 // implementation of primaryKey should support composite keys (unique across those properties)
  keys: [
    {name: 'location_id', primaryKey: true, type: 'location'}, // OR {name: 'location_id', values: [101, 102, 6, ...]}
    {name: 'year_id', primaryKey: true, type: 'year', start: 1990, end: 1995}, 
    {name: 'sex_id', primaryKey: true, type: 'sex'}, 
    {name: 'mean', type: 'random', initial: 200, end: 400}
  ],
  length: 20, // amount of datum objects
  uncertainty: 5, // percent of initial value
  volatility: 1 // greater numbers indicate greater variation in data (nice to have, not necessary)
});

The propTypes declaration for Layers is missing the required "visible" field.

It should be applied to the outermost, wrapping <g> within which <Symbol />s are rendered. symbolStyle should be exposed as a new prop and sent down into each <Symbol />.

Hi,

I was looking for a d3 brush component which works with React and found your project. I don't have any interactivity for range-slider demo. Also, no callback on Brush to get the updated range. Just wonder about any plan to expand this component.

Your project looks very clean and of high quality. Thanks for open sourcing it.
Cheers,
bsr

Steps to reproduce:

1. Click "Toggle subnational"
2. Resize window (notice the choropleth seems to zoom in)
3. Click "Toggle subnational" again
4. Resize window again

Results:

The choropleth disappears and console error:

Error: <path> attribute d: Expected number, "MNaN,NaNLNaN,NaNL…".          DOMPropertyOperations.js:152 

For sake of consistency, <Slider />'s wrapperClassName should be renamed className and wrapperStyle should be renamed style.

Should group be given a better default? I see the potential for key collisions if two ExpansionContainers (that both relied on the defaultProp "default" for group) are included on a page at once.

colorSteps: https://github.com/ihmeuw/ihme-ui/blob/docs/src/utils/colors.js#L9

Evaluate https://github.com/ihmeuw/ihme-ui/blob/master/src/ui/axis/src/axis.jsx#L159.

defaultProp of d3.scaleLinear() obviates need for call to atLeastOneofProp

We should think through how to make these abstractions clearer to read and reason about.

May be better to pull this out of the render method.