Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Remove "context" prop from visualizations #4789

Merged
merged 2 commits into from
Apr 13, 2020
Merged

Remove "context" prop from visualizations #4789

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0499696
Remove context from visualizations renderer
gabrieldutra Apr 8, 2020
08f0cbb
Add paginationSize to table's Default options
gabrieldutra Apr 12, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 11 additions & 2 deletions client/app/visualizations/components/EditVisualizationDialog.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { isEqual, extend, map, sortBy, findIndex, filter, pick } from "lodash";
import { isEqual, extend, map, sortBy, findIndex, filter, pick, omit } from "lodash";
import React, { useState, useMemo, useRef, useEffect } from "react";
import PropTypes from "prop-types";
import Modal from "antd/lib/modal";
Expand Down Expand Up @@ -125,7 +125,16 @@ function EditVisualizationDialog({ dialog, visualization, query, queryResult })

function save() {
setSaveInProgress(true);
const visualizationData = extend(newVisualization(type), visualization, { name, options, query_id: query.id });
let visualizationOptions = options;
if (type === "TABLE") {
visualizationOptions = omit(visualizationOptions, ["paginationSize"]);
}

const visualizationData = extend(newVisualization(type), visualization, {
name,
options: visualizationOptions,
query_id: query.id,
});
saveVisualization(visualizationData).then(savedVisualization => {
updateQueryVisualizations(query, savedVisualization);
dialog.close(savedVisualization);
Expand Down
9 changes: 7 additions & 2 deletions client/app/visualizations/components/VisualizationRenderer.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,11 +57,17 @@ export default function VisualizationRenderer(props) {
const { showFilters, visualization } = props;
const { Renderer, getOptions } = registeredVisualizations[visualization.type];

let options = getOptions(visualization.options, data);

// define pagination size based on context for Table visualization
if (visualization.type === "TABLE") {
options.paginationSize = props.context === "widget" ? "small" : "default";
}

// Avoid unnecessary updates (which may be expensive or cause issues with
// internal state of some visualizations like Table) - compare options deeply
// and use saved reference if nothing changed
// More details: https://github.com/getredash/redash/pull/3963#discussion_r306935810
let options = getOptions(visualization.options, data);
if (isEqual(lastOptions.current, options)) {
options = lastOptions.current;
}
Expand All @@ -85,7 +91,6 @@ export default function VisualizationRenderer(props) {
options={options}
data={filteredData}
visualizationName={visualization.name}
context={props.context}
/>
</div>
</ErrorBoundary>
Expand Down
1 change: 0 additions & 1 deletion client/app/visualizations/prop-types.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ export const RendererPropTypes = {
data: Data.isRequired,
options: VisualizationOptions.isRequired,
onOptionsChange: PropTypes.func, // (newOptions) => void
context: PropTypes.oneOf(["query", "widget"]).isRequired,
};

// For each visualization's editor
Expand Down
4 changes: 2 additions & 2 deletions client/app/visualizations/table/Renderer.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { filter, map, initial, last, reduce } from "lodash";
import { filter, map, get, initial, last, reduce } from "lodash";
import React, { useMemo, useState, useRef, useCallback, useEffect } from "react";
import Table from "antd/lib/table";
import Input from "antd/lib/input";
Expand Down Expand Up @@ -120,7 +120,7 @@ export default function Renderer({ options, data, context }) {
columns={tableColumns}
dataSource={preparedRows}
pagination={{
size: context === "widget" ? "small" : "",
size: get(options, "paginationSize", ""),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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" }}>

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. it should be mentioned (with default value) in getOptions;
  2. 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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Done;
  2. 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).

position: "bottom",
pageSize: options.itemsPerPage,
hideOnSinglePage: true,
Expand Down
1 change: 1 addition & 0 deletions client/app/visualizations/table/getOptions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import { clientConfig } from "@/services/auth";

const DEFAULT_OPTIONS = {
itemsPerPage: 25,
paginationSize: "default", // not editable through Editor
};

function getColumnContentAlignment(type) {
Expand Down