Remove "context" prop from visualizations #4789
Conversation
| @@ -120,7 +120,7 @@ export default function Renderer({ options, data, context }) { | |||
| columns={tableColumns} | |||
| dataSource={preparedRows} | |||
| pagination={{ | |||
| size: context === "widget" ? "small" : "", | |||
| size: get(options, "paginationSize", ""), | |||
There was a problem hiding this comment.
@gabrieldutra That's definitely a right direction, but let's improve it even more. Let's not extent visualization's options, but instead pass a new prop (customOptions, rendererOptions, whatever) of type object. It will contain additional settings like paginationSize. VisualizationRenderer should also take this prop and just pass it to the Renderer.
And usage in widget component: <VisualizationRenderer ...(existing props) customOptions={{ paginationSize: "small" }}>
There was a problem hiding this comment.
@kravets-levko I thought about that too, but somehow that's still a layout option for the Table visualization, although we don't offer it as a setting. So why not keep it it simple and leave it in the unified place for options instead of creating a new prop?
There was a problem hiding this comment.
I have some concerns about this solution.
First of all, visualization options object should contain only options that could be edited by user (not necessarily right now, we might have some "hidden" options that aren't available in UI, but they behave like other ones). That's not good if we introduce some "magic" option that comes from somewhere else than corresponding "getOptions" function. Furthermore, it's 2x bad when that option (which in fact depends on state of app that uses library) comes from a wrapper component where it's hard-coded and depends on visualization type. Therefore I suggested a new prop.
Regarding
which in fact depends on state of app that uses library
Pagination size depends on where visualization is used in the app, so in fact we still keep that "context" concept, but call it in a different way (we directly map context to paginationSize).
That's what my idea solves: if app needs to change pagination size, then app should tell about it. No any magic and hardcode in the library. Also, some of other visualizations have pagination as well (e.g. Details), so that custom option may be used there (BTW - good chance to implement it now 🙂). And the app is the only right place to decide when we need small pagination - library should just do what the app says.
There was a problem hiding this comment.
I'll give some more time to think about that, but first I'll give you more details on why I went that way.
When thinking in isolated Renderer and Editor, there's not much of an "could be edited by user" concept. I was thinking of the Renderer by being a "you give the Type, the Data and the Options and it renders your component" (so in here the "options" seemed to just be the right place for it). Also there shouldn't be a problem on having this option on getOptions (and it probably should be there, it's just that I forgot to add it).
The way I was trying to think about it was: In an isolated, non-Redash app, what would be the way to provide the paginationSize as a setting. The "static" part of it is a Redash thing, thus there's the "hack" of making it a static option over the renderer options.
I see your point though, I'll give it some time to think, if it's really necessary I'll introduce a renderOptions or staticOptions in there too. I just don't want to give complexity on the Library use as I see it can be confusing since there's no clear difference between that new option and the "options" itself when you forget about the Redash use for it.
There was a problem hiding this comment.
Thank you, now I see your point. I think we could end up with something between: for me it looks more or less fine to pass paginationSize among visualization options, BUT:
- it should be mentioned (with default value) in
getOptions; - there should be no special handling for it (I mean that IF in VisualizationRenderer). Let's move this logic upper, to Widget component - let widget extends visualization options with that new value and then passes it to VisualizationRenderer.
WDYT?
There was a problem hiding this comment.
- Done;
- It does seem to make sense, the problem is how things are structured now. VisualizationRenderer is the first component that is on the Redash app side, currently it takes a visualization object (not a data and options separately), so to extend it in there wee need to change the visualization object or refactor component props.
Currently EditVisualizationDialog and VisualizationRenderer are the ones on the Redash side that are abstracting visualizations for Redash use. I hope those become clearer once the separating process is done (and so we can rethink about how to structure such abstractions).
What type of PR is this? (check all applicable)
Description
Use a Table visualization option instead (it was the only lib that used that prop)
Related Tickets & Documents
--
Mobile & Desktop Screenshots/Recordings (if there are UI changes)
--