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

276 error using boosted 4 tooltip in an angular project #407

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

Conversation

jacques-lebourgeois
Copy link
Member

Related issues

Error using boosted 4 tooltip in an Angular projec

Description

I just pushed two commits:

  • One for the bug: the bug isn't really solved here, but worked around. I don't know how to use the boosted global object when the packaging system used seems to make the boosted variable inaccessible. I'm interested in solutions. In the meantime, I'm doing a fallback on the popover and tooltip renderer using Apache Echarts customized to the ODS charter.
  • The second commit is an attempt to improve the documentation to make it more readable and fluid.

Motivation & Context

Cf bug and its comments

Types of change

  • Bug fix (non-breaking which fixes an issue)
  • Doc enhancement

Test checklist

Please check that the following tests projects are still working:

  • docs/examples
  • test/angular-ngx-echarts
  • test/angular-echarts
  • test/html
  • test/react
  • test/vue
  • test/examples/bar-line-chart
  • test/examples/single-line-chart
  • test/examples/timeseries-chart

Copy link

netlify bot commented Nov 15, 2024

Deploy Preview for ods-charts ready!

Name Link
🔨 Latest commit e2f4f9a
🔍 Latest deploy log https://app.netlify.com/sites/ods-charts/deploys/674089086ca42700084c6a1e
😎 Deploy Preview https://deploy-preview-407--ods-charts.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@jacques-lebourgeois jacques-lebourgeois marked this pull request as ready for review November 15, 2024 13:21
@jacques-lebourgeois jacques-lebourgeois force-pushed the 276-error-using-boosted-4-tooltip-in-an-angular-project branch from 4d5a3bc to 494bf83 Compare November 22, 2024 11:18
@jacques-lebourgeois jacques-lebourgeois force-pushed the 276-error-using-boosted-4-tooltip-in-an-angular-project branch from 494bf83 to 6f84bd4 Compare November 22, 2024 11:21
* @returns returns back the theme manager object
* @param popoverConfig the configuration of the externalizePopover feature {@link ODSChartsPopoverConfig}
* @param popoverDefinition renderer {@link ODSChartsPopoverDefinition} of the popover/tooltip. **{@link ODSChartsPopoverManagers}** is used to
* specify preconfigured renderer for Apache ECharts, Boosted 5 or Boosted 4 renderer.
Copy link
Member Author

Choose a reason for hiding this comment

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

Add a warning that using Boosted 4 or Boosted 5 rendering requires dependency on the boosted library and access to the boosted global variable.

* - ODSChartsPopoverManagers.BOOSTED4: to use Boosted 4 tooltip/popover
* - `ODSChartsPopoverManagers.NONE`: to use default ECharts template to externalize tooltip/popover HTML element, implementing Orange Design System
* - `ODSChartsPopoverManagers.BOOSTED5`: to use Boosted 5 tooltip/popover
* - `ODSChartsPopoverManagers.BOOSTED4`: to use Boosted 4 tooltip/popover
Copy link
Member Author

Choose a reason for hiding this comment

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

Add a warning that using Boosted 4 or Boosted 5 rendering requires dependency on the boosted library and access to the boosted global variable.

@jacques-lebourgeois
Copy link
Member Author

I think this PR corrects the bug as expressed.

I've added a warning about the dependency on the Boosted library for Popover and Tooltip rendering.

Copy link
Member

@louismaximepiton louismaximepiton left a comment

Choose a reason for hiding this comment

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

Some small changes to the doc, but the PR already looks good.
Do we agree that none of the files in ./test/angular-ng-boosted was changed ?

@@ -90,6 +91,11 @@ export class ODSChartsPopoverDefinition {
* getPopupTemplate() may be specify to replace a specific template for the popup/tooltip to replace the default one
*/
public getPopupTemplate?: (categoryLabel: string, tooltipElements: ODSChartsPopoverItem[]) => string;
/**
* if provided, the `testAvailibility()` will be called to check if this renderer is available.
* If not, the fall back is en empty `ODSChartsPopoverDefinition`, ie `ODSChartsPopoverManagers.NONE`
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* If not, the fall back is en empty `ODSChartsPopoverDefinition`, ie `ODSChartsPopoverManagers.NONE`
* If not, the fall back is an empty `ODSChartsPopoverDefinition`, ie `ODSChartsPopoverManagers.NONE`

@@ -186,6 +192,21 @@ export class ODSChartsPopoverConfig {
}

class BOOSTED5_Definition extends ODSChartsPopoverDefinitionWithRenderer {
public testAvailibility = (): boolean => {
let availbility = true;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
let availbility = true;
let availability = true;

@@ -221,6 +242,21 @@ class BOOSTED5_Definition extends ODSChartsPopoverDefinitionWithRenderer {
}

class BOOSTED4_Definition extends ODSChartsPopoverDefinitionWithRenderer {
public testAvailibility = (): boolean => {
let availbility = true;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
let availbility = true;
let availability = true;

@@ -73,6 +73,8 @@ myChart.setOption(themeManager.getChartOptions(dataOptions));

The `themeManager` returned by `getThemeManager()` can be used to add other features supported by the ODS Charts library.

Read the [API documentaion](https://ods-charts.netlify.app/api/) for details on this legends, popover, tooltip... features.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Read the [API documentaion](https://ods-charts.netlify.app/api/) for details on this legends, popover, tooltip... features.
Read the [API documentation](https://ods-charts.netlify.app/api/) for details on this legends, popover, tooltip... features.

@@ -23,7 +23,7 @@
"serve:any": "serve",
"serve:react": "serve test/react/build/",
"serve:vue": "serve test/vue/dist/",
"typedoc": "typedoc --out docs/api src/ods-charts.ts --disableSources --excludePrivate --excludeProtected --readme docs/README.md && node build/add_head.mjs",
"typedoc": "typedoc --out docs/api src/ods-charts.ts --skipErrorChecking --disableSources --excludePrivate --excludeProtected --readme docs/README.md && node build/add_head.mjs",
Copy link
Member

Choose a reason for hiding this comment

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

Not a big fan of this, I'd rather have warning during the compilation, and maybe create a new issue around these.

* @returns returns back the theme manager object
* @param echart the initialized eCharts object
* @param legendHolderSelector
* legendHolderSelectorcan be
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* legendHolderSelectorcan be
* legendHolderSelector can be:

* @param echart the initialized eCharts object
* @param legendHolderSelector
* legendHolderSelectorcan be
* - a string, and then is interprated as the CSS selector of the legends container
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* - a string, and then is interprated as the CSS selector of the legends container
* - a string, and then is interpreted as the CSS selector of the legends container

* {@link ODSChartsPopoverManagers} gives preconfigured renderer
* @param popoverConfig the configuration of the externalizePopover feature {@link ODSChartsPopoverConfig}
* @param popoverDefinition renderer {@link ODSChartsPopoverDefinition} of the popover/tooltip. **{@link ODSChartsPopoverManagers}** is used to
* specify preconfigured renderer for Apache ECharts, Boosted 5 or Boosted 4 renderer.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* specify preconfigured renderer for Apache ECharts, Boosted 5 or Boosted 4 renderer.
* specify preconfigured renderer for Apache ECharts, Boosted 5 or Boosted 4.

*
* default value is {@link ODSChartsPopoverManagers.NONE}: uses default Apache ECharts template to externalize tooltip/popover HTML element, implementing Orange Design system
* Default value is {@link ODSChartsPopoverManagers.NONE}, that means it uses default Apache ECharts template to externalize tooltip/popover HTML element, implementing Orange Design system.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* Default value is {@link ODSChartsPopoverManagers.NONE}, that means it uses default Apache ECharts template to externalize tooltip/popover HTML element, implementing Orange Design system.
* Default value is {@link ODSChartsPopoverManagers}.NONE, that means it uses default Apache ECharts template to externalize tooltip/popover HTML element, implementing Orange Design system.

The link seems to be broken, not sure my workaround works in here.

* optionally you can use this call to set dataOptions
* @returns the Apache ECharts options
* @param dataOptions optionally you can use this call to set dataOptions, if not already set.
* @returns the Apache ECharts options to use in [Apache Echarts setOption()](https://echarts.apache.org/en/option.html) call.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* @returns the Apache ECharts options to use in [Apache Echarts setOption()](https://echarts.apache.org/en/option.html) call.
* @returns the Apache ECharts options to use in [Apache Echarts `setOption()`](https://echarts.apache.org/en/option.html) call.

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.

Error using boosted 4 tooltip in an Angular project
2 participants