synchro-charts icon indicating copy to clipboard operation
synchro-charts copied to clipboard

Feature/dial

Open jiaoyangChen opened this issue 2 years ago • 29 comments

Overview

Provide an explanation of what the change is

Tests

[Include a link to the passing GitHub action running the test suite here.]

Legal

This project is available under the Apache 2.0 License.

jiaoyangChen avatar Jul 13 '22 10:07 jiaoyangChen

This pull request introduces 1 alert when merging 65fefedcf4b2ad29db645bcc292f77f22636cbb0 into 5407d84072fdcbaf2becff23238485b7f59348fb - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

lgtm-com[bot] avatar Jul 15 '22 09:07 lgtm-com[bot]

This pull request introduces 1 alert when merging 70034156d6fc930f2c8842cf490ab27c5aeed7d5 into 5407d84072fdcbaf2becff23238485b7f59348fb - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

lgtm-com[bot] avatar Jul 18 '22 06:07 lgtm-com[bot]

To use the parameters dataStream and associatedStreams rather than dataStreams property, I changed the input type of 'sc-dial/sc-dial.tsx' file.

jiaoyangChen avatar Jul 18 '22 06:07 jiaoyangChen

also make sure yarn run test passes : https://github.com/awslabs/synchro-charts/runs/7548619663?check_suite_focus=true

diehbria avatar Jul 27 '22 21:07 diehbria

make sure to have commits squashed, to have a 1 to 1 mapping of PR to commits

diehbria avatar Jul 27 '22 21:07 diehbria

This pull request introduces 1 alert when merging 8df1a47373824c435e21840429dc6842e0195c0e into 5407d84072fdcbaf2becff23238485b7f59348fb - view on LGTM.com

new alerts:

  • 1 for Useless conditional

lgtm-com[bot] avatar Aug 09 '22 11:08 lgtm-com[bot]

This pull request introduces 1 alert when merging c7ccbb69ebaf4e4731232adbb3cc6a9565aab92f into 5407d84072fdcbaf2becff23238485b7f59348fb - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

lgtm-com[bot] avatar Aug 16 '22 13:08 lgtm-com[bot]

This pull request introduces 1 alert when merging 8ee4f1588e5ab36eff9f0976eb0f7fce0df9564c into 5407d84072fdcbaf2becff23238485b7f59348fb - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

lgtm-com[bot] avatar Aug 16 '22 13:08 lgtm-com[bot]

PR doesn't build,

on build:

[ ERROR ]  TypeScript: ./src/components/charts/common/types.ts:9:3
           Module '"../../../utils/dataTypes"' has no exported member 'DialSizeConfig'.

      L8:  DataStreamId,
      L9:  DialSizeConfig,
     L10:  MessageOverrides,

Please make sure the branch builds and i'll take a closer look

diehbria avatar Aug 17 '22 15:08 diehbria

We've added some coding guidelines at https://github.com/awslabs/iot-app-kit/blob/main/docs/CodingGuidelines.md which should help on setting expectations and general guidelines. please take a look

diehbria avatar Aug 17 '22 15:08 diehbria

some comments left un-responded to - and build still fails. please ensure build passes

Hi diehbria,

Please check the procedures below:

First, we will package synchro-charts and generate dist.file, then the newly generated dist.file will replace the existing dist.file in synchro-charts-react: node_modules/@synchro-charts/core. And then you can build successfully via yarn build.

We can build the synchro chart successfully in our local environment. If you want us to test the build in the AWS environment, please provide the authorization to us, we are happy to offer any support you need.

RayWangCN avatar Aug 19 '22 12:08 RayWangCN

the build fails because the generated typescript type definitions are not present in the commit:

Run yarn build
yarn run v1.22.19
$ lerna run build --stream
lerna notice cli v4.0.0
lerna info ci enabled
lerna info Executing command in 3 packages: "yarn run build"
@synchro-charts/react: $ tsc --outDir dist
@synchro-charts/core: $ stencil build
@synchro-charts/core: [33:32.5]  @stencil/core
@synchro-charts/core: [33:32.7]  v1.17.4 🏏
Error: @synchro-charts/react: src/components.ts(19,59): error TS2[6](https://github.com/awslabs/synchro-charts/runs/7975789303?check_suite_focus=true#step:8:7)94: Namespace 'LocalJSX' has no exported member 'ScDial'.
Error: @synchro-charts/react: src/components.ts(19,6[7](https://github.com/awslabs/synchro-charts/runs/7975789303?check_suite_focus=true#step:8:8)): error TS2304: Cannot find name 'HTMLScDialElement'.
@synchro-charts/react: error Command failed with exit code 2.
@synchro-charts/react: info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
lerna ERR! yarn run build exited 2 in '@synchro-charts/react'
lerna WARN complete Waiting for 1 child process to exit. CTRL-C to exit immediately.
error Command failed with exit code 2.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
Error: Process completed with exit code 2.Run yarn build
yarn run v1.22.19
$ lerna run build --stream
lerna notice cli v4.0.0
lerna info ci enabled
lerna info Executing command in 3 packages: "yarn run build"
@synchro-charts/react: $ tsc --outDir dist
@synchro-charts/core: $ stencil build
@synchro-charts/core: [33:32.5]  @stencil/core
@synchro-charts/core: [33:32.7]  v1.17.4 🏏
Error: @synchro-charts/react: src/components.ts(19,59): error TS2[6](https://github.com/awslabs/synchro-charts/runs/7975789303?check_suite_focus=true#step:8:7)94: Namespace 'LocalJSX' has no exported member 'ScDial'.
Error: @synchro-charts/react: src/components.ts(19,6[7](https://github.com/awslabs/synchro-charts/runs/7975789303?check_suite_focus=true#step:8:8)): error TS2304: Cannot find name 'HTMLScDialElement'.
@synchro-charts/react: error Command failed with exit code 2.
@synchro-charts/react: info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
lerna ERR! yarn run build exited 2 in '@synchro-charts/react'
lerna WARN complete Waiting for 1 child process to exit. CTRL-C to exit immediately.
error Command failed with exit code 2.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
Error: Process completed with exit code 2.

please add this file into the repo, and it won't fail due to missing typescript types

diehbria avatar Aug 23 '22 15:08 diehbria

the build fails because the generated typescript type definitions are not present in the commit:

Run yarn build
yarn run v1.22.19
$ lerna run build --stream
lerna notice cli v4.0.0
lerna info ci enabled
lerna info Executing command in 3 packages: "yarn run build"
@synchro-charts/react: $ tsc --outDir dist
@synchro-charts/core: $ stencil build
@synchro-charts/core: [33:32.5]  @stencil/core
@synchro-charts/core: [33:32.7]  v1.17.4 🏏
Error: @synchro-charts/react: src/components.ts(19,59): error TS2[6](https://github.com/awslabs/synchro-charts/runs/7975789303?check_suite_focus=true#step:8:7)94: Namespace 'LocalJSX' has no exported member 'ScDial'.
Error: @synchro-charts/react: src/components.ts(19,6[7](https://github.com/awslabs/synchro-charts/runs/7975789303?check_suite_focus=true#step:8:8)): error TS2304: Cannot find name 'HTMLScDialElement'.
@synchro-charts/react: error Command failed with exit code 2.
@synchro-charts/react: info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
lerna ERR! yarn run build exited 2 in '@synchro-charts/react'
lerna WARN complete Waiting for 1 child process to exit. CTRL-C to exit immediately.
error Command failed with exit code 2.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
Error: Process completed with exit code 2.Run yarn build
yarn run v1.22.19
$ lerna run build --stream
lerna notice cli v4.0.0
lerna info ci enabled
lerna info Executing command in 3 packages: "yarn run build"
@synchro-charts/react: $ tsc --outDir dist
@synchro-charts/core: $ stencil build
@synchro-charts/core: [33:32.5]  @stencil/core
@synchro-charts/core: [33:32.7]  v1.17.4 🏏
Error: @synchro-charts/react: src/components.ts(19,59): error TS2[6](https://github.com/awslabs/synchro-charts/runs/7975789303?check_suite_focus=true#step:8:7)94: Namespace 'LocalJSX' has no exported member 'ScDial'.
Error: @synchro-charts/react: src/components.ts(19,6[7](https://github.com/awslabs/synchro-charts/runs/7975789303?check_suite_focus=true#step:8:8)): error TS2304: Cannot find name 'HTMLScDialElement'.
@synchro-charts/react: error Command failed with exit code 2.
@synchro-charts/react: info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
lerna ERR! yarn run build exited 2 in '@synchro-charts/react'
lerna WARN complete Waiting for 1 child process to exit. CTRL-C to exit immediately.
error Command failed with exit code 2.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
Error: Process completed with exit code 2.

please add this file into the repo, and it won't fail due to missing typescript types

@jiaoyangChen is taking care of this and it will be resolved once the other code changes.

We should be aware feedback is not being avoided when a new commit comes in. Ji is working to resolve all feedback and is not intending for new commits to be complete revisions ready for publishing.

tracy-french avatar Aug 25 '22 00:08 tracy-french

This pull request introduces 1 alert when merging b4e2bba3701265943749fff001888a0c123d69cb into 5407d84072fdcbaf2becff23238485b7f59348fb - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

lgtm-com[bot] avatar Aug 26 '22 06:08 lgtm-com[bot]

This pull request fixes 1 alert when merging e3ff0e10c14b45bdd4affffb08bcdd34b39f2aaf into 5407d84072fdcbaf2becff23238485b7f59348fb - view on LGTM.com

fixed alerts:

  • 1 for Unused or undefined state property

lgtm-com[bot] avatar Aug 26 '22 07:08 lgtm-com[bot]

This pull request introduces 1 alert and fixes 1 when merging ceadb2507ea18808834ddcee05f8262684f81695 into 5407d84072fdcbaf2becff23238485b7f59348fb - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

fixed alerts:

  • 1 for Unused or undefined state property

lgtm-com[bot] avatar Aug 26 '22 09:08 lgtm-com[bot]

This pull request fixes 1 alert when merging b74315acf11e8cac4f5dfc87bf7eb43effe337f4 into 5407d84072fdcbaf2becff23238485b7f59348fb - view on LGTM.com

fixed alerts:

  • 1 for Unused or undefined state property

lgtm-com[bot] avatar Aug 26 '22 09:08 lgtm-com[bot]

This pull request fixes 1 alert when merging 2e5b9cf06b4b682b68e09322dca3e3f20561b13b into 5407d84072fdcbaf2becff23238485b7f59348fb - view on LGTM.com

fixed alerts:

  • 1 for Unused or undefined state property

lgtm-com[bot] avatar Aug 26 '22 10:08 lgtm-com[bot]

Please re-request review when everything is ready for review, so I know everything is addressed and ready for comment.

diehbria avatar Aug 26 '22 15:08 diehbria

This pull request fixes 1 alert when merging a8b1c28403f2f19fed51a2cfd37da8881981c0cd into 5407d84072fdcbaf2becff23238485b7f59348fb - view on LGTM.com

fixed alerts:

  • 1 for Unused or undefined state property

lgtm-com[bot] avatar Aug 29 '22 07:08 lgtm-com[bot]

This pull request fixes 1 alert when merging a8b1c28 into 5407d84 - view on LGTM.com

fixed alerts:

  • 1 for Unused or undefined state property

This is the problem with merging the main code, and I had no idea about it.

jiaoyangChen avatar Aug 29 '22 08:08 jiaoyangChen

Please re-request review when everything is ready for review, so I know everything is addressed and ready for comment.

I guess the last error is there are some problems about script test:typescript.

jiaoyangChen avatar Aug 29 '22 09:08 jiaoyangChen

This pull request fixes 1 alert when merging d7e3974ff0e00298cc18c9dbad9bc3eda6282923 into 5407d84072fdcbaf2becff23238485b7f59348fb - view on LGTM.com

fixed alerts:

  • 1 for Unused or undefined state property

lgtm-com[bot] avatar Aug 30 '22 03:08 lgtm-com[bot]

This pull request fixes 1 alert when merging 35f4e9665f319c3534a5cedb80a860fdeaf7b625 into 5407d84072fdcbaf2becff23238485b7f59348fb - view on LGTM.com

fixed alerts:

  • 1 for Unused or undefined state property

lgtm-com[bot] avatar Aug 31 '22 03:08 lgtm-com[bot]

please clean up the git history - we want one pull request to add one additional commit to the tip of what is on the main branch. If there's some way to break up the PR so we can get code in, and avoid one mega PR that has everything, that would be ideal. It's difficult to review one large PR.

diehbria avatar Aug 31 '22 22:08 diehbria

This pull request fixes 1 alert when merging 8d70434e82cc173f94520d8780f694356158c3e8 into 5407d84072fdcbaf2becff23238485b7f59348fb - view on LGTM.com

fixed alerts:

  • 1 for Unused or undefined state property

lgtm-com[bot] avatar Sep 02 '22 06:09 lgtm-com[bot]

This pull request fixes 1 alert when merging 293311e758a3923de73f76d25633ae9e7c4ce792 into 5407d84072fdcbaf2becff23238485b7f59348fb - view on LGTM.com

fixed alerts:

  • 1 for Unused or undefined state property

lgtm-com[bot] avatar Sep 02 '22 06:09 lgtm-com[bot]

This pull request fixes 1 alert when merging cbd66a60cad2911e30b27409a5810b90bfba2338 into 5407d84072fdcbaf2becff23238485b7f59348fb - view on LGTM.com

fixed alerts:

  • 1 for Unused or undefined state property

lgtm-com[bot] avatar Sep 02 '22 07:09 lgtm-com[bot]

please clean up the git history - we want one pull request to add one additional commit to the tip of what is on the main branch. If there's some way to break up the PR so we can get code in, and avoid one mega PR that has everything, that would be ideal. It's difficult to review one large PR.

Since there was no module when modifying the code before, there was no way to split it, so I re-committed all the functions of Dial. The PR will be closed.

Then the gauge will be submitted according to the module, including basic components, automatic test, document three parts.

jiaoyangChen avatar Sep 02 '22 09:09 jiaoyangChen

closing this one since work has moved over to https://github.com/awslabs/synchro-charts/pull/161

diehbria avatar Sep 09 '22 17:09 diehbria