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

fix(dashboard v2): fix some bugs #1084

Merged
merged 6 commits into from
Mar 20, 2022
Merged

fix(dashboard v2): fix some bugs #1084

merged 6 commits into from
Mar 20, 2022

Conversation

cloudcarver
Copy link
Contributor

What's changed and what's your intention?

  1. Fix the following bugs:
    • The canvas is not re-rendered when the view point is changed by mouse instead of trackpad.
    • Graph is blank when choosing to show just mview instead of the whole graph.
    • Routing error when refreshing the page.
    • The canvas is not re-rendered while zooming.
  2. Add some comments
  3. Add an npm script to build and test the dashboard with risingwave in local development environment.

Checklist

  • I have written necessary docs and comments
  • I have added necessary unit tests and integration tests

@cloudcarver cloudcarver added the type/fix Bug fix label Mar 20, 2022
@cloudcarver cloudcarver requested a review from skyzh March 20, 2022 05:57
@cloudcarver
Copy link
Contributor Author

Still reading docs to deal with 404 fallback in the axum backend. These are some essential issues affecting common usage.

Copy link
Contributor

@skyzh skyzh left a comment

Choose a reason for hiding this comment

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

Rest LGTM, good work!

@@ -4,7 +4,8 @@
"dev": "next dev",
"build": "next build",
"start": "next start",
"build-static": "next export"
"build-static": "next export",
"build-prod": "next export && rm -rf ../.risingwave/ui && rm -rf ui && cp -r out ui && mv ui ../.risingwave"
Copy link
Contributor

@skyzh skyzh Mar 20, 2022

Choose a reason for hiding this comment

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

I'd prefer not to hardcode the path for PREFIX_UI. Maybe update Makefile.toml with a new task:

e.g.

[tasks.export-dashboard-v2]
script = """
rm -rf "${PREFIX_UI}"
cd dashboard && npm run build-prod
cd .. && ln -s dashboard/out ${PREFIX_UI}
"""

and we write...

"build-prod": "next export"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure. I'll take it look :)

Copy link
Contributor

Choose a reason for hiding this comment

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

... wait a minute, I think any change to PREFIX_UI directory will be over-written by tasks.extract-dashboard-artifact...

What about having a Build dashboard from source option in riselab-config? I might add that next week.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. It will be over-written by risedev. Currently we need to run build-prod after risedev is activated. Using Build dashboard from source is more convenient.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

emm, seems building dashboard from source is not so convenient for development. Since every time we want to test with meta node, we need to shut down the risingwave and reboot it, and then run some tests.... It is better to have a script which can replace the current built artifact in runtime.

Copy link
Contributor

Choose a reason for hiding this comment

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

We can use first use ./risedev d to start the cluster (which calls export-dashboard-v2 for the first time). Then, we may use ./risedev export-dashboard-v2 to rebuild dashboard, without restarting the meta node. Does this sound good?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wonderful!

Copy link
Contributor

Choose a reason for hiding this comment

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

Implemented in #1086, you make take a look :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. I think it is convenient enough now :)

@@ -36,6 +36,18 @@ The website will be served at port 3000.
npm run dev
```

## Test with risingwave meta node
Copy link
Contributor

Choose a reason for hiding this comment

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

Please also help removing Dashboard section in README.md, and instruct users to take a look at the README.md under dashboard folder.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay!

@cloudcarver cloudcarver enabled auto-merge (squash) March 20, 2022 07:42
@cloudcarver cloudcarver merged commit 75574c0 into main Mar 20, 2022
@cloudcarver cloudcarver deleted the mikecw/dashboard branch March 20, 2022 07:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/fix Bug fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants