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

documentation improvements (typos, links, images) #731

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

savvar9991
Copy link

The image is restored
Fixed a typo
Improved Ivezual display
replaced non -working link to the correct

@savvar9991 savvar9991 requested review from gregnazario, a team, hariria and igor-p as code owners December 8, 2024 12:46
Copy link

vercel bot commented Dec 8, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
developer-docs-nextra ❌ Failed (Inspect) Dec 11, 2024 7:07pm

Copy link
Collaborator

@gregnazario gregnazario left a comment

Choose a reason for hiding this comment

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

Generally good, but there are a few updates where changes to img tags don't make sense

Comment on lines 16 to 24
<a target="_blank" href="https://github.com/aptos-labs/aptos-ts-sdk">
![Github Repo Stars](https://img.shields.io/github/stars/aptos-labs/aptos-ts-sdk)
<img src="https://img.shields.io/github/stars/aptos-labs/aptos-ts-sdk" alt="Github Repo Stars" />
</a>
<a target="_blank" href="https://www.npmjs.com/package/@aptos-labs/ts-sdk">
![NPM Version](https://img.shields.io/npm/v/%40aptos-labs%2Fts-sdk)
<img src="https://img.shields.io/npm/v/%40aptos-labs%2Fts-sdk" alt="NPM Version" />
</a>
{/* Revisit later if we need these */}
{/* <a target="_blank" href="https://www.npmjs.com/package/@aptos-labs/ts-sdk">
![Node Version](https://img.shields.io/node/v/%40aptos-labs%2Fts-sdk)
</a> */}
{/* <a target="_blank" href="https://www.npmjs.com/package/@aptos-labs/ts-sdk">
![NPM bundle size](https://img.shields.io/bundlephobia/min/%40aptos-labs/ts-sdk)
</a> */}
<a target="_blank" href="https://aptos-labs.github.io/aptos-ts-sdk/@aptos-labs/ts-sdk-latest">
![Static Badge](https://img.shields.io/badge/SDK_Reference-Docs)
<img src="https://img.shields.io/badge/SDK_Reference-Docs" alt="SDK Reference" />
</a>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, these seem to work right now, any reason to change from markdown to HTML?

Copy link
Author

Choose a reason for hiding this comment

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

MDX format is specifically designed to combine the simplicity of Markdown with the power of HTML where needed. In this case, using HTML tags in MDX is a valid and more suitable solution for creating interactive badges
Screenshot before changes:
image
Screenshot after changes:
image
The HTML version provides:

  • Proper handling of external links with target="_blank"
  • Better visual presentation of badges

Comment on lines 21 to 23
export function DynamicImage({ title }) {
const imageUrl = `${docsConfig.origin}/api/og.png?title=${encodeURIComponent(title)}`;

return (
<img src={imageUrl} alt="Dynamic Image" />
);
}

<DynamicImage title="Dynamic Image" />
<img
src="https://assets-global.website-files.com/606f63778ec431ec1b930f1f/63dbd502218a274f2a602968_aptos.png"
alt="Example Open Graph Image"
style="width: 100%; max-width: 800px; height: auto; border: 1px solid #eaeaea; border-radius: 8px; margin: 20px 0"
/>
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is supposed to be a dynamic image example, so it doesn't make much sense here to change to an img

Copy link
Author

Choose a reason for hiding this comment

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

You're right about this being a dynamic image example. I made these changes because the dynamic image generation wasn't working due to server issues (the server needs to be running with @vercel/og package and proper Edge Runtime setup for OG image generation to work).

I'll create a separate issue to track and fix the server setup required for dynamic image generation. For now, I'll revert these changes to keep the original dynamic image example in the documentation.
image

![v-fn-network.svg](/docs/v-fn-network.svg)
![v-fn-network.svg](/apps/nextra/public/docs/v-fn-network.svg)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This image is currently working? I think it should stay how it is

Copy link
Author

Choose a reason for hiding this comment

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

Actually, the image with the original path isn't working - it shows as a broken image. The new path I added points to the correct location of the file in the project structure (/apps/nextra/public/docs/v-fn-network.svg), which makes the image display properly.

Would you like me to show you the difference between how it looks with both paths?
Screenshot before changes:
image
Screenshot after changes:
image

Comment on lines 80 to 88
##### Generate Favicon
### Generate Favicon

Navigate to [https://favicon.io/favicon-converter/](https://favicon.io/favicon-converter/).
Upload an `svg` file with the logo you'd like to generate your favicon for.

##### Copy assets into `public/favicon`
### Copy assets into `public/favicon`

After generating the assets, favicon.ico should return a zip file containing
After generating the assets, favicon.io will return a zip file containing:

- `android-chrome-192x192.png`
- `favicon.ico`
- ...so on and so forth
- And other favicon assets

Copy all of these and replace the existing files in `public/favicon`
Copy all of these files and replace the existing files in `public/favicon`

##### Deploy
### Deploy
Copy link
Collaborator

Choose a reason for hiding this comment

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

These should be 4 #, as they're part of the favicon section, but I'll accept 3 for now, as it'll match above.

#### Generate Favicon

Reverted changes back to the original dynamic image generation example to maintain proper documentation of OG image functionality. 

The static image was temporarily added due to server setup issues, but it's better to keep the correct implementation in the docs even if it requires additional server configuration to work locally.

Related issue will be created to track server setup requirements.
Copy link
Author

@savvar9991 savvar9991 left a comment

Choose a reason for hiding this comment

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

I've addressed the feedback:

  1. For node-networks-sync.mdx: Fixed the image path to correctly display the network diagram (provided before/after screenshots showing the fix)
  2. For ts-sdk.mdx: Will keep the original dynamic image implementation and create a separate issue for server setup requirements

Comment on lines 16 to 24
<a target="_blank" href="https://github.com/aptos-labs/aptos-ts-sdk">
![Github Repo Stars](https://img.shields.io/github/stars/aptos-labs/aptos-ts-sdk)
<img src="https://img.shields.io/github/stars/aptos-labs/aptos-ts-sdk" alt="Github Repo Stars" />
</a>
<a target="_blank" href="https://www.npmjs.com/package/@aptos-labs/ts-sdk">
![NPM Version](https://img.shields.io/npm/v/%40aptos-labs%2Fts-sdk)
<img src="https://img.shields.io/npm/v/%40aptos-labs%2Fts-sdk" alt="NPM Version" />
</a>
{/* Revisit later if we need these */}
{/* <a target="_blank" href="https://www.npmjs.com/package/@aptos-labs/ts-sdk">
![Node Version](https://img.shields.io/node/v/%40aptos-labs%2Fts-sdk)
</a> */}
{/* <a target="_blank" href="https://www.npmjs.com/package/@aptos-labs/ts-sdk">
![NPM bundle size](https://img.shields.io/bundlephobia/min/%40aptos-labs/ts-sdk)
</a> */}
<a target="_blank" href="https://aptos-labs.github.io/aptos-ts-sdk/@aptos-labs/ts-sdk-latest">
![Static Badge](https://img.shields.io/badge/SDK_Reference-Docs)
<img src="https://img.shields.io/badge/SDK_Reference-Docs" alt="SDK Reference" />
</a>
Copy link
Author

Choose a reason for hiding this comment

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

MDX format is specifically designed to combine the simplicity of Markdown with the power of HTML where needed. In this case, using HTML tags in MDX is a valid and more suitable solution for creating interactive badges
Screenshot before changes:
image
Screenshot after changes:
image
The HTML version provides:

  • Proper handling of external links with target="_blank"
  • Better visual presentation of badges

Comment on lines 21 to 23
export function DynamicImage({ title }) {
const imageUrl = `${docsConfig.origin}/api/og.png?title=${encodeURIComponent(title)}`;

return (
<img src={imageUrl} alt="Dynamic Image" />
);
}

<DynamicImage title="Dynamic Image" />
<img
src="https://assets-global.website-files.com/606f63778ec431ec1b930f1f/63dbd502218a274f2a602968_aptos.png"
alt="Example Open Graph Image"
style="width: 100%; max-width: 800px; height: auto; border: 1px solid #eaeaea; border-radius: 8px; margin: 20px 0"
/>
Copy link
Author

Choose a reason for hiding this comment

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

You're right about this being a dynamic image example. I made these changes because the dynamic image generation wasn't working due to server issues (the server needs to be running with @vercel/og package and proper Edge Runtime setup for OG image generation to work).

I'll create a separate issue to track and fix the server setup required for dynamic image generation. For now, I'll revert these changes to keep the original dynamic image example in the documentation.
image

![v-fn-network.svg](/docs/v-fn-network.svg)
![v-fn-network.svg](/apps/nextra/public/docs/v-fn-network.svg)
Copy link
Author

Choose a reason for hiding this comment

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

Actually, the image with the original path isn't working - it shows as a broken image. The new path I added points to the correct location of the file in the project structure (/apps/nextra/public/docs/v-fn-network.svg), which makes the image display properly.

Would you like me to show you the difference between how it looks with both paths?
Screenshot before changes:
image
Screenshot after changes:
image

@gregnazario
Copy link
Collaborator

Okay, I get it now, you're looking at GitHub, rather than the website aptos.dev directly. Mainly focused on readability there, so will need to double check on the preview site

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.

3 participants