-
Notifications
You must be signed in to change notification settings - Fork 130
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
Version 2.1! #204
base: main
Are you sure you want to change the base?
Version 2.1! #204
Conversation
64dde07
to
e56eb2e
Compare
New Features: - More modular design - Less dependencies/complexity - Style sheets imported into Shadow DOM - Draws with (Chart)Wrapper - Introduces: - Dashboards - Controls - Queries - Editors - Call methods on charts - Chart Actions - Heavily updated demos Some known issues: - P0 Needs more tests! - P1 gViz bug? Drawing a WordTree shows an error - P3 gViz bug? Changing chart type has slow style sheet update due to delayed ready event.
<template> | ||
<style> | ||
:host { | ||
display: inline-block; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
usually components that change display
property of :host
should also have:
:host([hidden]) {
display: none;
}
display: none; | ||
} | ||
</style> | ||
<div id="control">Loading control...</div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should users of the component be able to translate this string?
/** | ||
* The label of the column in the data to control. | ||
* Either `label` or `index` should be set, not both. | ||
* @type {string} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@type {?string}
string
is non-nullable
}, | ||
/** | ||
* The state of the specific control. | ||
* @type {Object|undefined} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@type {!Object|undefined}
Object
is nullable
}, | ||
/** | ||
* Specifies the group for the chart in a Dashboard. | ||
* @type {string} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@type {string|undefined}
property does not have a default value
}, | ||
_v: new GoogleChartLoader().visualization, | ||
_onDataChanged(data) { | ||
this.fire('google-chart-data-change', data); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this event does not have any documentation
can the event from notify: true
property be used instead?
@rslawik Thanks for the review! |
@wesalvaro I'm guessing this would all have to be redone now that the code has been migrated to LitElement? |
We added few things from the list:
Not sure if we want to pursue the modular approach ( However, adding support for dashboards / controls / editors / action may still be worth exploring. Would those be useful to you? |
New Features:
Some known issues: