Disable / Enable attrs based on custom config #956
Conversation
|
this is the commit i'd like to double check my logic on: 6f8473f I think what I've done is the right thing to do, but maybe we could discuss this tomorrow |
|
So when excluding color specifically we want to be able to say "yes to column mapping but no to specific color choosing" e.g.
|
|
Similarly we want to be able to say "constant vs variable" but just hide the color-picker widget:
|
|
maybe we need to do widget-type-matching. like color-picker and font-family. |
|
what happened to the image dropzone thing? |
|
I don't know if it's just atm that it's slow, but we just went from 60 snapshots to 252, so these tests will be taking significantly longer now.. |
src/lib/unpackPlotProps.js
Outdated
| return false; | ||
| } | ||
|
|
||
| export function computeCustomConfigVisibility(props, fullValue, customConfig, wrappedComponent) { |
There was a problem hiding this comment.
rename to notHiddenByCustomConfig ?
There was a problem hiding this comment.
or notHiddenByCustomVisibilityRules ?
There was a problem hiding this comment.
but the outcome of this function can be true or false for visibility
There was a problem hiding this comment.
the "positive" version of this is permittedByCustomVisibilityRules
|
I think I addressed all the comments, please let me know if I can merge and release @nicolaskruchten. |
|
|
||
| it('HIDES Field based on customConfig', () => | ||
| expect(wrapper.find('input').length).toEqual(0)); | ||
| it('SHOWS PlotlySection if it has an attr that is accepted by customConfig', () => |
There was a problem hiding this comment.
shouldn't this section be hidden because it's empty?
There was a problem hiding this comment.
I could make it so, if i remove a part of the current logic that we have: https://github.com/plotly/react-chart-editor/blob/master/src/components/containers/PlotlySection.js#L51
(the || , just make it return if there's no child)
It didn't seem necessary for the current config we're trying to get right, and it's previously existing logic, that is not entirely false. (I mean it appears because we did put an attr, that is accepted by customConfig on a PlotlySection)
There was a problem hiding this comment.
to make this test easier to understand, can we make it such that there is another widget in the section with the same attr as the section?
|
💃 |
I don't think I want to quite include the full whitelisting functionality in this pr. It can be a foundation for it, but more thought would have to be put into it, and it's not really the core of the functionality that we need atm.
Just to show an example of a case, say you enable very few attributes (just 'colorway' for example). Then you get situations like this:
Where you have a panel that can add annotations, but nothing can be edited. It seems odd, feels like that menu item should not even be there. Thoughts @nicolaskruchten ?