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

Version 2.1! #204

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Version 2.1! #204

wants to merge 2 commits into from

Conversation

wesalvaro
Copy link
Member

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.

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;
Copy link
Contributor

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;
}

best practices

display: none;
}
</style>
<div id="control">Loading control...</div>
Copy link
Contributor

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}
Copy link
Contributor

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}
Copy link
Contributor

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}
Copy link
Contributor

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);
Copy link
Contributor

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?

google-chart-editor.js Show resolved Hide resolved
@wesalvaro
Copy link
Member Author

@rslawik Thanks for the review!

@moderndeveloperllc
Copy link

@wesalvaro I'm guessing this would all have to be redone now that the code has been migrated to LitElement?

@rslawik
Copy link
Contributor

rslawik commented Jul 19, 2020

We added few things from the list:

  • Style sheets imported into Shadow DOM
  • Draws with (Chart)Wrapper

Not sure if we want to pursue the modular approach (<google-chart-data>). That would be a big breaking change.

However, adding support for dashboards / controls / editors / action may still be worth exploring. Would those be useful to you?

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