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.

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

Visualizations refactor #100

Merged
merged 11 commits into from
Mar 4, 2014
Merged

Visualizations refactor #100

merged 11 commits into from
Mar 4, 2014

Conversation

arikfr
Copy link
Member

@arikfr arikfr commented Feb 15, 2014

I've separated each visualization declaration into its own module, so each visualization defines how to render it and what options it needs. The base visualization render/edit directives are not aware of the visualization types that are available.

Adding a new visualization requires:

  1. Write the module.
  2. Add the module to the redash.app module dependencies.
  3. Include the file in the JS files list in index.html.

I would love if it could be a single step, but not sure how to make #2 and #3 automatic (specially #3).

Would love to get comments and ideas.

@joeysim
Copy link
Contributor

joeysim commented Feb 15, 2014

Re 2 and 3, I'd think of it from a convention over configuration angle, where we have a folder with visualization packages, each in its own folder and exposes enough meta data to be properly integrated. When re:dash loads, it will enumerate this folder and allow access to the different visualizations.

In such a model, you would init and render visualizations lazily and on-demand rather than include all when re:dash web page loads.

wdyt?

@arikfr
Copy link
Member Author

arikfr commented Feb 16, 2014

It will require some effort in "orchestrating" this, but it's possible:

  1. While developing we can use some Jinja template helper that will load all files in /visualizations folder.
  2. Configure grunt to "build" (uglify, minify, ...) all files in /visualizations as part of the build process.

This doesn't solve #2 though, but I will RTFM to see if there is some easy solution around it.

Also there is the question of how to handle external dependencies visualizations might have, but for now I would "skip" this and address when needed.

@arikfr
Copy link
Member Author

arikfr commented Feb 16, 2014

Ok, #2 is practically solved (see: 1a896e9).


this.visualizations[type] = visualization;

if (!skipTypes) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@arikfr when would we like to skip types?

Copy link
Member Author

Choose a reason for hiding this comment

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

In the case of table.

Copy link
Contributor

Choose a reason for hiding this comment

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

why? isn't that a valid vis. type?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's created by default, so we don't need it in the menu.

The main code doesn't know about individual visualizations and each visualization is contained in its own module. Should make adding/editing/removing visualizations easier.
this.visualizations = {};
this.visualizationTypes = {};

this.registerVisualization = function(type, name, rendererTemplate, editorTemplate, defaultOptions, skipTypes) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@arikfr how about using a config object here?

this.registerVisualization = function(config) {
  var visualization = {
   rendererTemplate: config.rendererTemplate,
   editorTemplate: config.editorTemplate,
   type: config.type,
   name: config.name,
   defaultOptions: config.defaultOptions || {}
  };
}

Copy link
Member Author

Choose a reason for hiding this comment

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

👍

Copy link
Member Author

Choose a reason for hiding this comment

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

@amirnissim
Copy link
Contributor

Good to merge 👍

amirnissim added a commit that referenced this pull request Mar 4, 2014
@amirnissim amirnissim merged commit cc957cc into master Mar 4, 2014
christophervalles added a commit to hailocab/redash that referenced this pull request Mar 6, 2014
christophervalles pushed a commit to hailocab/redash that referenced this pull request May 8, 2014
@arikfr arikfr deleted the feature_visualization_options branch September 11, 2014 04:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants