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

Synching up my origin with most recent code #8

Conversation

Alex-NRCan
Copy link
Member

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

Simplified the Chart type usage.
Started work on the WCAG legend instead of using the canvas.
Created a GeoChartAction principle to handle the redrawing from inside the component (instead of bothering with setTimeout from the caller).


This change is Reviewable

@Alex-NRCan Alex-NRCan self-assigned this Oct 17, 2023
@Alex-NRCan Alex-NRCan requested a review from jolevesq October 17, 2023 16:23
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/chart.tsx line 212 at r1 (raw file):

            else if (ds.backgroundColor) color = ds.backgroundColor! as string;

            // Return the Legent item

Typo Legent


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

      return (
        <div style={style} className={styles.chartContainer}>
          <div className={styles.chartContainerGrid1}>{renderDatasetSelector()}</div>

Instead of css, can you use the MUI grid system?

We should use sxClasses and Box element to follow MUI?


src/chart-validator.ts line 41 at r1 (raw file):

                  type: 'array',
                  items: {
                    type: 'integer',

Should it be number?


src/chart-validator.ts line 50 at r1 (raw file):

                    properties: {
                      x: {
                        type: 'integer',

Should it be number?


src/chart-validator.ts line 53 at r1 (raw file):

                      },
                      y: {
                        type: 'integer',

Should it be number?


src/chart-validator.ts line 68 at r1 (raw file):

                {
                  type: 'string',
                  // pattern:

Can you put a description tag to put the info: Accept hexadecimal and rgba (i.e. ff000000 or rgba(255,55,255,1) instead of the whole pattern. At the end, this file may be use to genarate form for user to fill and will use description tag. It will be easier for user to understand.


src/chart-validator.ts line 129 at r1 (raw file):

          chart: {
            type: 'string',
            pattern: '^(line|bar|pie|doughnut)$',

Put an enum instead of string

@Alex-NRCan Alex-NRCan force-pushed the feat-validator-action-cleanup-of-types branch from 49c1301 to d62f02d Compare October 17, 2023 20:10
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: 6 of 10 files reviewed, 7 unresolved discussions (waiting on @Alex-NRCan and @jolevesq)


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

Previously, jolevesq (Johann Levesque) wrote…

Typo Legent

Ok


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

Previously, jolevesq (Johann Levesque) wrote…

Instead of css, can you use the MUI grid system?

We should use sxClasses and Box element to follow MUI?

Ok


src/chart-validator.ts line 41 at r1 (raw file):

Previously, jolevesq (Johann Levesque) wrote…

Should it be number?

Ok


src/chart-validator.ts line 50 at r1 (raw file):

Previously, jolevesq (Johann Levesque) wrote…

Should it be number?

Ok


src/chart-validator.ts line 53 at r1 (raw file):

Previously, jolevesq (Johann Levesque) wrote…

Should it be number?

Ok


src/chart-validator.ts line 68 at r1 (raw file):

Previously, jolevesq (Johann Levesque) wrote…

Can you put a description tag to put the info: Accept hexadecimal and rgba (i.e. ff000000 or rgba(255,55,255,1) instead of the whole pattern. At the end, this file may be use to genarate form for user to fill and will use description tag. It will be easier for user to understand.

Yeah.. actually.. I ended up getting rid of the pattern (and left it there in case), because I also wanted to support color names like: 'red', 'blue' (which are indeed supported) and so I didn't want to create a whole dictionary of colors via a regex(!). So I've let it there like this for now.. I could remove the comment altogether I guess?


src/chart-validator.ts line 129 at r1 (raw file):

Previously, jolevesq (Johann Levesque) wrote…

Put an enum instead of string

Good point

@Alex-NRCan Alex-NRCan force-pushed the feat-validator-action-cleanup-of-types branch from f77a49e to dca3d77 Compare October 17, 2023 20:14
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 1 of 9 files at r1, 1 of 4 files at r2.
Reviewable status: 7 of 10 files reviewed, 4 unresolved discussions (waiting on @Alex-NRCan)


src/chart-validator.ts line 68 at r1 (raw file):

Previously, Alex-NRCan (Alex) wrote…

Yeah.. actually.. I ended up getting rid of the pattern (and left it there in case), because I also wanted to support color names like: 'red', 'blue' (which are indeed supported) and so I didn't want to create a whole dictionary of colors via a regex(!). So I've let it there like this for now.. I could remove the comment altogether I guess?

I think you can remove. If you add the description tag it will explain what it support. From the string, try to create the color and if it fails, throw an error. If the input is wrong, throw error


src/chart-validator.ts line 129 at r1 (raw file):

Previously, Alex-NRCan (Alex) wrote…

Good point

Type must be enum and put your default to line as it is the default in the code.

@Alex-NRCan Alex-NRCan force-pushed the feat-validator-action-cleanup-of-types branch from f89f91a to e229df1 Compare October 17, 2023 20:24
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 1 of 4 files at r2, all commit messages.
Reviewable status: 8 of 10 files reviewed, 2 unresolved discussions (waiting on @Alex-NRCan)


src/chart.tsx line 236 at r3 (raw file):

                  defaultChecked
                />
                <Typography style={{ color: color as string }} sx={sxClasses.checkDataset} noWrap>

You can style directly in the sx like sx'{[sxClasses.check, { color: color]}}


src/chart-validator.ts line 129 at r1 (raw file):

Previously, jolevesq (Johann Levesque) wrote…

Type must be enum and put your default to line as it is the default in the code.

"chart": {
          "enum": ["line", "bar", ...],
          "default": "line",
          "description": "Supported type of chart."

        }

@Alex-NRCan Alex-NRCan force-pushed the feat-validator-action-cleanup-of-types branch from f1d7baa to ccbb0ad Compare October 17, 2023 20:51
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 10 files reviewed, 2 unresolved discussions (waiting on @jolevesq)


src/chart.tsx line 236 at r3 (raw file):

Previously, jolevesq (Johann Levesque) wrote…

You can style directly in the sx like sx'{[sxClasses.check, { color: color]}}

Done.


src/chart-validator.ts line 68 at r1 (raw file):

Previously, jolevesq (Johann Levesque) wrote…

I think you can remove. If you add the description tag it will explain what it support. From the string, try to create the color and if it fails, throw an error. If the input is wrong, throw error

Done.


src/chart-validator.ts line 129 at r1 (raw file):

Previously, jolevesq (Johann Levesque) wrote…

Type must be enum and put your default to line as it is the default in the code.

Done.

Simplified the Chart type usage.
Started work on the WCAG legend instead of using the canvas.
Adjusted the chart validator schema
Using Grid and Box now instead of html div
Removed chart.module.css and removed Globals.d.ts
Added comment
Removed patterns
Added default value for the enum for the validator
Merged the style and the sx
@Alex-NRCan Alex-NRCan force-pushed the feat-validator-action-cleanup-of-types branch from 31a3097 to f2fa322 Compare October 17, 2023 20:54
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 1 of 2 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @Alex-NRCan)

@jolevesq jolevesq merged commit 96c5b2e into Canadian-Geospatial-Platform:develop Oct 17, 2023
github-actions bot pushed a commit that referenced this pull request Oct 17, 2023
…-action-cleanup-of-types

Synching up my origin with most recent code (#8)
@Alex-NRCan Alex-NRCan deleted the feat-validator-action-cleanup-of-types branch October 18, 2023 16:26
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.

2 participants