-
Notifications
You must be signed in to change notification settings - Fork 143
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
feat (toolbar):Debug toolbar #686
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The toolbar looks good overall, I left some comments.
Seems that we're close to be able to merge this 👍
Also, it seems you had an example usage of the toolbar in web-wa-sqlite example but you removed it? |
820892e
to
a166ab4
Compare
@kevin-dp I've pretty done everything as you suggest. A few notes: I had to add export { MockRegistry } from './mock' to satellite/index.ts to get the import to work without the addition export is the package I couldn't refactor the tab components as you suggested, I had tried to do this before, its something to do with when react expands out the JSX, it generates react hook errors. I have moved the whole thing for now - but we can do this later I haven't added back the toolbar in an example as I want to do this an other step once there is a published toolbar package to refer to properly. But I have checked that this will works ok in the web example. I have also added a non compiled |
toolbar/.prettierignore
Outdated
*.md | ||
.DS_Store | ||
dist/ | ||
dev-app/dist/ |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
toolbar/src/index.tsx
Outdated
|
||
function ElectricToolbar({ api }: ToolbarProps) { | ||
const [hidden, setHidden] = useState(true) | ||
const [dbNames, setDbNames] = useState(['']) |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
@kevin-dp I think this might be ready now - I've made to readme changes you suggested |
9cd7775
to
dd63265
Compare
I see there is still activity here. Let's not leave it hanging. |
d95e3aa
to
7f0e2f4
Compare
@kevin-dp I have done all the changes you suggested except the additional tests, and I have tested it manually against 0.0.9 with the web example. Can we merge this now as I'm not really coding now and don't have time to think about how to set up the extra tests and Valter wanted this in for next week? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for addressing my comments!
I have two nitpicks that would be nice to fix:
components/toolbar/Makefile
indentation is still offclients/typescript/src/satellite/index.ts
export is still on the block of imports
You tested the toolbar manually on 0.9 and it seems to work which is great. However, we don't have unit tests for some public methods and that would be a blocker for me as that means we will have to go and manually test the toolbar on every release. These are the untested public methods:
getSatelliteStatus(name: string): ConnectivityState | 'Not found'
resetDB(dbName: string): Promise<void>
queryDB(dbName: string, statement: Statement): Promise<Row[]>
I will let @balegas decide whether we merge without the tests or not.
Hi @paulharter, This PR is dragging for too long. It's very near. Best, |
4b2abcf
to
88b21e5
Compare
4bc6f8a
to
4017938
Compare
I've modified the debug toolbar slightly, added a couple more APIs for the connection status (toggle on/off) and an experimental shapes tab for viewing active shapes. The last one uses a private field from the satellite implementation so before developing further I'll open a PR to expose some shape subscription information from the Satellite client for the debug toolbar to use safely. I've also done an ugly inlining of CSS into the debug toolbar to make the DX better (now only need to dynamically import and call @kevin-dp can you have another look so we can potentially merge this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me!
Left some minor comments.
@@ -0,0 +1,9 @@ | |||
deps: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indentation is still very weird in this file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what you mean? it's tab indented (similar to the generator Makefile)
951c638
to
302f016
Compare
Checked out toolbar stuff into this new clean branch as many many messy changes of mind.