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

Added the steps native examples #37

Conversation

Alex-NRCan
Copy link
Member

@Alex-NRCan Alex-NRCan commented Oct 20, 2023

Added the chart line steps native examples
Made chart-validator more generic with a schema-validator
Suggestion to enforcing return types via ESLint
Moved the schema validators into json files instead of in the code

Fixes #6 and #7

Link to try it here: https://alex-nrcan.github.io/geochart/


This change is Reviewable

@Alex-NRCan Alex-NRCan requested a review from jolevesq October 20, 2023 15:57
@Alex-NRCan Alex-NRCan self-assigned this Oct 20, 2023
Copy link
Member

@jolevesq jolevesq left a comment

Choose a reason for hiding this comment

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

Reviewed 7 of 9 files at r1, all commit messages.
Reviewable status: 7 of 9 files reviewed, 7 unresolved discussions (waiting on @Alex-NRCan)


src/app.tsx line 44 at r1 (raw file):

    // Show the error (actually, can't because the snackbar is linked to a map at the moment
    // and geochart is standalone without a cgpv.init() at all)

Snackbar is in ui, you can't use it without a map?


src/chart.tsx line 241 at r1 (raw file):

   */
  const renderChartContainerFailed = (): JSX.Element => {
    return <div style={{ fontStyle: 'italic', color: 'red' }}>Error rendering the Chart. Check console for details.</div>;

In tsx component it is better to use the mui component (Box in this case) and sx classes


src/schema-validator.ts line 66 at r1 (raw file):

  /**
   * Returns a string representation of the errors of all ValidatorResult objects.
   */

Mostly for public function, can we add JSDOC @param and @return values


src/schema-validator-data.json line 6 at r1 (raw file):

  "description": "This Schema validator validates the GeoChart data.",
  "type": "object",
  

Remove trailing white spaces


src/schema-validator-data.json line 9 at r1 (raw file):

  "properties": {
    "labels": {
      "description": "The labels to use for the X axis.",

Should we have labels: { x: 'my x', y: 'my y' }. It is more clear for use to fill instead of array. This can be optional and only use for bar and line chart


src/schema-validator-data.json line 25 at r1 (raw file):

          },
          "data": {
            "oneOf": [

Is the type of dataset link to the type of chart?


src/schema-validator-data.json line 52 at r1 (raw file):

            ]
          },
          "backgroundColor": {

Does style attribute should be in the data object and get ride of the array possibility.... one date, one style

Copy link
Member

@jolevesq jolevesq left a comment

Choose a reason for hiding this comment

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

Reviewable status: 7 of 9 files reviewed, 7 unresolved discussions (waiting on @Alex-NRCan)


src/schema-validator-data.json line 52 at r1 (raw file):

Previously, jolevesq (Johann Levesque) wrote…

Does style attribute should be in the data object and get ride of the array possibility.... one date, one style

forget about this one, it is encapsulated in one datataset... so why the array of border?

Copy link
Member

@jolevesq jolevesq left a comment

Choose a reason for hiding this comment

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

Reviewable status: 7 of 9 files reviewed, 7 unresolved discussions (waiting on @Alex-NRCan)


src/schema-validator-data.json line 9 at r1 (raw file):

Previously, jolevesq (Johann Levesque) wrote…

Should we have labels: { x: 'my x', y: 'my y' }. It is more clear for use to fill instead of array. This can be optional and only use for bar and line chart

If we do not have specific block of schema by type of chart, it make sense to have the array but we would need good description tag to explain

@Alex-NRCan Alex-NRCan force-pushed the feat-validator-and-return-types branch 2 times, most recently from 680cbb6 to 5497765 Compare October 20, 2023 17:43
Copy link
Member Author

@Alex-NRCan Alex-NRCan left a comment

Choose a reason for hiding this comment

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

Reviewable status: 5 of 9 files reviewed, 7 unresolved discussions (waiting on @Alex-NRCan, @jolevesq, @param, and @return)


src/app.tsx line 44 at r1 (raw file):

Previously, jolevesq (Johann Levesque) wrote…

Snackbar is in ui, you can't use it without a map?

The function to show a Snackbar message is like: "cgpv.api.utilities.showError" which needs a 'mapId' as first parameter to raise an event that the Snackbar listens on, but I don't have a mapId (and I believe there's no Snackbar listening on anything, because cgpv was never initialized?).

If you're suggesting that I 'require' a Snackbar and build it for the template application, I could try that instead of calling the 'official' method to show messages of the api.


src/chart.tsx line 241 at r1 (raw file):

Previously, jolevesq (Johann Levesque) wrote…

In tsx component it is better to use the mui component (Box in this case) and sx classes

True. I need to lose my 'div' reflexes. I've updated the PR.


src/schema-validator.ts line 66 at r1 (raw file):

Previously, jolevesq (Johann Levesque) wrote…

Mostly for public function, can we add JSDOC @param and @return values

Yes. I've updated the PR.


src/schema-validator-data.json line 6 at r1 (raw file):

Previously, jolevesq (Johann Levesque) wrote…

Remove trailing white spaces

Yes. I've updated the PR.


src/schema-validator-data.json line 9 at r1 (raw file):

Previously, jolevesq (Johann Levesque) wrote…

If we do not have specific block of schema by type of chart, it make sense to have the array but we would need good description tag to explain

This is there, simply because it follows the Chart.js standard of being able to type 'generic' labels in an easy, optional way.
One can also specify axis labels in another, more custom, way too.
This generic "labels" property is optional and depends on what kind of chart you're dealing with indeed.


src/schema-validator-data.json line 25 at r1 (raw file):

Previously, jolevesq (Johann Levesque) wrote…

Is the type of dataset link to the type of chart?

Some data will be more compatible with some types yes and will display weirdly in other charts.
We can start exploding the data various possibilities by chart types if we have to.


src/schema-validator-data.json line 52 at r1 (raw file):

Previously, jolevesq (Johann Levesque) wrote…

forget about this one, it is encapsulated in one datataset... so why the array of border?

I'm not sure I understand your question, we can talk about it

Made chart-validator more generic with schema-validator
Enforcing return types
Moved the schema validators into json files
Added comments and cleanup
Now with 2 handles per slider
@Alex-NRCan Alex-NRCan force-pushed the feat-validator-and-return-types branch from 5497765 to 5dee0e9 Compare October 20, 2023 17:55
Copy link
Member

@jolevesq jolevesq left a comment

Choose a reason for hiding this comment

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

Reviewed 4 of 5 files at r2, all commit messages.
Reviewable status: 8 of 9 files reviewed, 3 unresolved discussions (waiting on @Alex-NRCan, @param, and @return)


src/app.tsx line 44 at r1 (raw file):

Previously, Alex-NRCan (Alex) wrote…

The function to show a Snackbar message is like: "cgpv.api.utilities.showError" which needs a 'mapId' as first parameter to raise an event that the Snackbar listens on, but I don't have a mapId (and I believe there's no Snackbar listening on anything, because cgpv was never initialized?).

If you're suggesting that I 'require' a Snackbar and build it for the template application, I could try that instead of calling the 'official' method to show messages of the api.

Ok so we have 3 choices:

  • make our utilities in geoview-core be use without at map at all
  • if chart is started from package, throw the error to the package and the package will start the snackbar
  • Create a new mechanisim in the geochart package

Copy link
Member Author

@Alex-NRCan Alex-NRCan left a comment

Choose a reason for hiding this comment

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

Reviewable status: 8 of 9 files reviewed, 3 unresolved discussions (waiting on @Alex-NRCan, @jolevesq, @param, and @return)


src/app.tsx line 44 at r1 (raw file):

Previously, jolevesq (Johann Levesque) wrote…

Ok so we have 3 choices:

  • make our utilities in geoview-core be use without at map at all
  • if chart is started from package, throw the error to the package and the package will start the snackbar
  • Create a new mechanisim in the geochart package

Yes. My comment in the code suggests that we could do something like point #1.

When you say the "package", are you talking about the geoview-geochart "plugin"?
If yes, then I'm already doing point #2 if you look at the geoview-geochart plugin (and no need for #3) :)

Copy link
Member

@jolevesq jolevesq left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 5 files at r2.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Alex-NRCan)

Copy link
Member

@jolevesq jolevesq left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @Alex-NRCan)

@jolevesq jolevesq merged commit e603783 into Canadian-Geospatial-Platform:develop Oct 23, 2023
github-actions bot pushed a commit that referenced this pull request Oct 23, 2023
…r-and-return-types

Added the steps native examples (#37)
@Alex-NRCan Alex-NRCan deleted the feat-validator-and-return-types branch October 23, 2023 13:42
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.

Work on the schema validator. Think of the default params in Chart.
2 participants