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

Add Getblock to Builder Tools #1023

Closed
wants to merge 15 commits into from

Conversation

JoGetBlock
Copy link

Getblock Polkadot RPC provider

👋👋 Hello there! Welcome. Please follow the instruction below.

Click the Preview tab and select a PR template:

@rphair rphair changed the title add Getblock to the tool kit Add Getblock to Builder Tools Apr 12, 2023
@rphair rphair added the builder tool Indicates a PR/issue on a builder tool label Apr 12, 2023
Copy link
Author

@JoGetBlock JoGetBlock left a comment

Choose a reason for hiding this comment

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

Thank you for the corrections

Copy link
Collaborator

@rphair rphair left a comment

Choose a reason for hiding this comment

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

I bookmarked this service 2 years ago after this was posted on the Cardano Forum: https://forum.cardano.org/t/api-connection-to-ada-nodes/58364

I thought 40K requests per day on a free tier was pretty good at the time but still have never had an opportunity to determine what this threshold would mean in practice. It seems this is going through Rosetta which for all I know might have been superseded in practice (and which is not unique to Cardano as I understand it): https://getblock.io/nodes/ada

  • (BTW I think this link would be a better landing page for the tool than the top level home page as currently appearing in the PR)

Compared with Blockfrost (see extensive Cardano specific documentation) there does not really seem anything Cardano specific here. On that home page in fact Cardano is not even listed among the top 8 supported blockchains.

In my opinion we might add this if @JoGetBlock & associates could add a getstarted: page with some orientation and tutorial for devs about how to validate this claim made in the description: is particularly true for Cardano:

GetBlock is a blockchain RPC provider that offers access to full Cardano nodes, allowing developers to build blockchain applications and services.

@rphair rphair requested a review from rdlrt April 12, 2023 11:36
@rphair
Copy link
Collaborator

rphair commented Apr 12, 2023

@JoGetBlock most importantly you would have to add the file ./builder-tools/getblock.png to this PR (i.e. in the same directory as the other Builder Tools images) for this request to proceed. As you can see, the Portal does not build without it. Logo images 2x as wide as they are high are recommended.

Copy link
Author

@JoGetBlock JoGetBlock left a comment

Choose a reason for hiding this comment

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

Added required changes

Copy link
Collaborator

@rphair rphair left a comment

Choose a reason for hiding this comment

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

@JoGetBlock the last changes were not "required" since, at this time:

  • the project image is still not included in this PR (preventing the Portal from even building, which you would see if you had done this locally as recommended here);
  • the getstarted: link is not quoted, generally a syntax error;
  • that link isn't what I was hoping to see in Add Getblock to Builder Tools #1023 (review) because
    • this page doesn't even mention Cardano;
    • what I was talking about: well integrated Builder Tools often write Get Started pages on the Dev Portal as an orientation for Cardano developers specifically... e.g. what Blockfrost has written here: https://developers.cardano.org/docs/get-started/blockfrost

I'll watch this while other reviewers have a look at this, but so far I believe little effort has been put into either creating a proper submission or establishing that this tool is part of the Cardano ecosystem.

@JoGetBlock
Copy link
Author

@JoGetBlock the last changes were not "required" since, at this time:

  • the project image is still not included in this PR (preventing the Portal from even building, which you would see if you had done this locally as recommended here);

  • the getstarted: link is not quoted, generally a syntax error;

  • that link isn't what I was hoping to see in Add Getblock to Builder Tools #1023 (review) because

    • this page doesn't even mention Cardano;
    • what I was talking about: well integrated Builder Tools often write Get Started pages on the Dev Portal as an orientation for Cardano developers specifically... e.g. what Blockfrost has written here: https://developers.cardano.org/docs/get-started/blockfrost

I'll watch this while other reviewers have a look at this, but so far I believe little effort has been put into either creating a proper submission or establishing that this tool is part of the Cardano ecosystem.

My mistake, didn't get this right, let me redo all these corrections once more and create a decent getstarted for you.

@rphair
Copy link
Collaborator

rphair commented Apr 27, 2023

@JoGetBlock had to touch up your code a bit because we were getting GitHub build check warnings because of the unquoted JS string fixed in 21a771a.

@rphair
Copy link
Collaborator

rphair commented Apr 27, 2023

p.s. @JoGetBlock note this still does not build because you will have to commit your project image file ./builder-tools/getblock.png to this branch.

@JoGetBlock
Copy link
Author

Added the image, thank you

@JoGetBlock
Copy link
Author

Also created a pull request to add the get started page #1056, not sure that this is how it's supposed to be done, but forgive me, I'm not from the development team.

Please, feel free to suggest any corrections

rphair
rphair previously requested changes May 14, 2023
Copy link
Collaborator

@rphair rphair left a comment

Choose a reason for hiding this comment

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

@JoGetBlock you are forgiven but you should find someone from your development team to help you do two important things: add file(s) to your PR, and resolve merge conflicts (required here at this time). Sometimes these things can be done on GitHub but more generally through the git command line in a local copy of your repository: something devs feel comfortable doing.

I'm closing #1056 for two reasons:

  1. the getstarted: page you have written depends upon this PR from being accepted, which does not seem definite;
  2. that page should be included in this PR, not an additional PR.

When including the getstarted you've proposed there, you should remove promotional language like this or rewrite it less promotionally:

The Cardano API endpoint by GetBlock is a useful tool for early-stage teams that want to take advantage of Cardano's benefits.

If you have any additional questions or would like to share your experience, feel free to join our community of Web3 developers who are always ready to chat.

... and you should definitely not include tricks to bump up your menu priority with Docusaurus headers (which nobody else is using) to promote your tool relative to others:

sidebar_position: 2

@JoGetBlock
Copy link
Author

@rphair created a new get-started page #1079

@rphair
Copy link
Collaborator

rphair commented Jun 7, 2023

As per #1079 (comment) I have closed the newer pull request because the new getstarted page as well as its missing update to the sidebars.js menu should be included here in this PR instead.

But first as we insisted in #1023 (review) you have to fix the merge conflict. GitHub itself can give you suggestions about how to do this if you hit the Resolve conflicts button below.

My guess is that you are not doing a local build of the Developer Portal, because otherwise your new code would be more easily included, tested & merged with what's here already. Can you please get someone with developer skills on your team to help you begin with these instructions:

... referring them first to what I've just posted? Then, once you get a working local build and commit your new code (builder tools addition + new getstarted page + sidebar entry), we can consider this submission based on its own merit.

@JoGetBlock
Copy link
Author

JoGetBlock commented Jun 19, 2023

@rphair Hi! Added changes to this PR, + added get-started, please take a look, thank you in advance

@rphair rphair dismissed their stale review June 21, 2023 08:48

portal builds now, but submission is still structurally flawed

@rphair
Copy link
Collaborator

rphair commented Jun 21, 2023

@JoGetBlock the portal builds correctly now but:

  • the added get-started.md page is not included in the navigational structure (see top level file sidebars.js)
  • it begins with a table rather than the standard Docusaurus header (see how other *.md files begin for the correct syntax)
  • it's in a subdirectory getblock which is inappropriate because there's only one file (it should be called getblock.md like all the other one-page contributions).

Given the repeated problems coming from inattention to the guidelines & consistency with other contributed material, I would be OK with closing this submission but I'll leave this to other reviewers.

@katomm
Copy link
Member

katomm commented Aug 1, 2023

Closing this but please if you want to fix the PR let me know and we can open it again.

@katomm katomm closed this Aug 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
builder tool Indicates a PR/issue on a builder tool
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants