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

Convert file to ts #7092

Merged
merged 8 commits into from
Jul 21, 2022
Merged

Convert file to ts #7092

merged 8 commits into from
Jul 21, 2022

Conversation

vdusart
Copy link
Contributor

@vdusart vdusart commented Jul 16, 2022

Moving files to TS.

Related Issue

Conversion to TS according to #6392

@gatsby-cloud
Copy link

gatsby-cloud bot commented Jul 16, 2022

Gatsby Cloud Build Report

ethereum-org-website-dev

🎉 Your build was successful! See the Deploy preview here.

Build Details

View the build logs here.

🕐 Build time: 11m

Copy link
Member

@pettinarip pettinarip left a comment

Choose a reason for hiding this comment

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

LGTM @vdusart 💪🏼 I've only added the Gatsby auto generated query types to the get-eth page. The rest is perfect.

@@ -1,18 +1,20 @@
// This content is used as a code example in /src/pages/index.js
// Kept here for easy formatting.
Copy link
Member

Choose a reason for hiding this comment

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

@samajammin from the comment I suspect that we shouldn't migrate this file to TS. Or maybe we can just delete it since it is not being used by other files. Let us know if you want to keep it as it is.

Sorry @vdusart for not detecting this question before. I know this file was on the list. Lets wait for Sam's confirmation before merging this 👍🏼

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@samajammin from the comment I suspect that we shouldn't migrate this file to TS. Or maybe we can just delete it since it is not being used by other files. Let us know if you want to keep it as it is.

Sorry @vdusart for not detecting this question before. I know this file was on the list. Lets wait for Sam's confirmation before merging this 👍🏼

Yes sure, no worries about that 😁

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You can find exactly the content of the CreateWallet.ts file on this page inside the A JavaScript Ethereum wallet code example, but it comes from this file...

Copy link
Member

Choose a reason for hiding this comment

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

That is right. From a conversation with Sam, let's keep this file in .js format as the plan is to import it in the code example block that you mentioned. See #7150 for more context.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh ok !
I'm gonna revert the changes on this file then and do the refactor to import the files 😄

Copy link
Member

Choose a reason for hiding this comment

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

Ok, cool. If possible lets do that refactor in a different PR so that we can merge this one earlier and to have things separated. What do you think?

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 of course, it doesn't really make sense to make the changes here. I asked sam to assign me the issue #7150 .

Copy link
Member

Choose a reason for hiding this comment

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

Perfect! thanks

@pettinarip pettinarip merged commit b5bfade into ethereum:dev Jul 21, 2022
@vdusart vdusart deleted the convert-file-to-TS branch July 21, 2022 15:52
@corwintines corwintines mentioned this pull request Jul 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
content 🖋️ This involves copy additions or edits
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants