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

migrate RPC Tool page #2270

Merged
merged 1 commit into from
Jan 3, 2024
Merged

migrate RPC Tool page #2270

merged 1 commit into from
Jan 3, 2024

Conversation

khancode
Copy link
Contributor

@khancode khancode commented Nov 17, 2023

Migrate RPC Tool page to Redocly. XRPL requests have been implemented but styles are a WIP.

@ckniffen
Copy link
Contributor

We call this an RPC tool but it is using websocket requests ...

Copy link

socket-security bot commented Dec 4, 2023

New dependencies detected. Learn more about Socket for GitHub ↗︎

Packages Version New capabilities Transitives Size Publisher
react18-json-view 0.2.6 eval +0 286 kB yysuni

@mDuo13
Copy link
Collaborator

mDuo13 commented Dec 5, 2023

I think a lot of times using JSON-RPC directly from a webpage hits CORS limitations, whereas WebSocket isn't subject to CORS (strange choice if you ask me, but that's what the browser vendors and/or W3C chose) so that's why it's WS and not JSON-RPC.

It's accurate if you view the WebSocket API as a "remote procedure call" API—it's certainly not a RESTful API—and JSON-RPC as a specific, different implementation of the XRPL's "RPC" APIs. But yeah, kind of confusing.

@intelliot
Copy link
Contributor

Good point @mDuo13 - given that, I think it's fine to call it RPC since we are doing RPC over WebSocket. The "JSON-RPC" implementation is over HTTP, so I think it's more clear to refer to that as the HTTP API.

@khancode khancode requested a review from mDuo13 December 13, 2023 19:19
}
}, []);

const getInfo = async (): Promise<void> => {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit please use functional syntax instead of const function syntax

async function getInfo(...)...

@@ -660,7 +660,7 @@
- group: Dev Tools
expanded: false
items:
- label: RPC Tool
- page: /dev-tools/rpc-tool.page.tsx
Copy link
Contributor

Choose a reason for hiding this comment

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

The label should be the same as before :)

Suggested change
- page: /dev-tools/rpc-tool.page.tsx
- label: RPC Tool
page: /dev-tools/rpc-tool.page.tsx

Comment on lines 139 to 147
setAccountInfoResponse(null);
setAccountLinesResponse(null);
setAccountTxResponse(null);
setAccountObjectsResponse(null);
setTxResponse(null);
setLedgerResponse(null);
setErrorText(null);
setShowResult(true);
setProgressBarWidth("0%");
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you group these variables with comments explaining the groupings? (Some of these seem like they should be broken out into sub-components, and some seem like separate parts of the page)

Copy link
Contributor

Choose a reason for hiding this comment

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

If you make sub-components, please add them to a sub-folder :)

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this has been done with my refactor.

Differences between original:
- react18-json-view used for responses
- cleaned up markup for expand and collapse

Original work on this was done by @khancode

Co-authored-by: Omar Khan <khancodegt@gmail.com>
@mDuo13 mDuo13 added this to the Toolchain-Migration-Redocly milestone Jan 3, 2024
@mDuo13 mDuo13 merged commit 65b7edc into XRPLF:redocly-migration Jan 3, 2024
Copy link
Collaborator

@mDuo13 mDuo13 left a comment

Choose a reason for hiding this comment

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

Approved (with a minor fix to the page width to compensate for the fact that Redocly's sidebar doesn't count as part of the 12 Bootstrap columns and another fix to add it to the sidebar)

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.

5 participants