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

Integrate docsite #396

Merged
merged 47 commits into from
Aug 3, 2022
Merged

Integrate docsite #396

merged 47 commits into from
Aug 3, 2022

Conversation

martriay
Copy link
Contributor

@martriay martriay marked this pull request as ready for review July 14, 2022 12:00
RELEASING.md Show resolved Hide resolved
RELEASING.md Outdated Show resolved Hide resolved
docs/modules/ROOT/nav.adoc Outdated Show resolved Hide resolved
docs/modules/ROOT/pages/index.adoc Outdated Show resolved Hide resolved
docs/modules/ROOT/pages/index.adoc Outdated Show resolved Hide resolved
docs/modules/ROOT/pages/accounts.adoc Outdated Show resolved Hide resolved
docs/modules/ROOT/pages/access.adoc Outdated Show resolved Hide resolved
docs/modules/ROOT/pages/access.adoc Show resolved Hide resolved
@martriay martriay marked this pull request as draft July 15, 2022 17:54
@martriay
Copy link
Contributor Author

martriay commented Jul 15, 2022

Pending items:

  • fix links: internal (within docs) and external (usually github)
  • fix API formatting (None.)

Future work, on other PRs:

  • splitting API docs into their own pages (opening the door to autogenerating them from natspec in the future)
  • automating version updates into antora.yml

Copy link
Collaborator

@andrew-fleming andrew-fleming left a comment

Choose a reason for hiding this comment

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

This is looking great! Aside from the already noted API formatting and links, I left a couple suggestions and threw in a potential solution regarding `None." for the APIs. And +1 to admonitions over blockquotes :)

docs/modules/ROOT/pages/erc20.adoc Outdated Show resolved Hide resolved
docs/modules/ROOT/pages/utilities.adoc Outdated Show resolved Hide resolved
docs/modules/ROOT/pages/proxies.adoc Outdated Show resolved Hide resolved
martriay and others added 2 commits July 22, 2022 15:18
Co-authored-by: Andrew Fleming <fleming.andrew@protonmail.com>
@martriay
Copy link
Contributor Author

martriay commented Jul 22, 2022

I'm not sure about mixing light with dark themes, since the dark one is better for code snippets surrounded by text. we could update only the API section but I guess it's better for a designer to look at it along with the rest of the docsite. It will also be easier to modify once we split the API from the rest of the docs, as with the Solidity docs.

This is how it the light theme looks in code blocks surrounded by text (ugly):
image

@martriay martriay marked this pull request as ready for review July 23, 2022 19:03
@martriay martriay marked this pull request as draft July 23, 2022 20:23
@martriay martriay marked this pull request as ready for review July 25, 2022 20:38
Copy link
Collaborator

@andrew-fleming andrew-fleming left a comment

Choose a reason for hiding this comment

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

This is looking great! I left a couple questions and a few comments :)

RELEASING.md Outdated Show resolved Hide resolved
docs/modules/ROOT/pages/accounts.adoc Outdated Show resolved Hide resolved
docs/modules/ROOT/pages/accounts.adoc Outdated Show resolved Hide resolved
docs/modules/ROOT/pages/access.adoc Show resolved Hide resolved
docs/modules/ROOT/pages/accounts.adoc Outdated Show resolved Hide resolved
martriay and others added 2 commits July 26, 2022 16:41
Co-authored-by: Andrew Fleming <fleming-andrew@protonmail.com>
@martriay martriay requested a review from andrew-fleming July 26, 2022 20:29
Copy link
Collaborator

@andrew-fleming andrew-fleming left a comment

Choose a reason for hiding this comment

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

Looks good to go!

Copy link
Contributor

@frangio frangio left a comment

Choose a reason for hiding this comment

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

Looking great. Minor comments:

docs/modules/ROOT/pages/index.adoc Outdated Show resolved Hide resolved
Comment on lines 82 to 90
* https://github.com/OpenZeppelin/cairo-contracts/blob/main/src/openzeppelin/account/Account.cairo[Account]
* https://github.com/OpenZeppelin/cairo-contracts/blob/main/tests/mocks/ERC165.cairo[ERC165]
* https://github.com/OpenZeppelin/cairo-contracts/blob/main/src/openzeppelin/token/erc20/presets/ERC20Mintable.cairo[ERC20Mintable]
* https://github.com/OpenZeppelin/cairo-contracts/blob/main/src/openzeppelin/token/erc20/presets/ERC20Pausable.cairo[ERC20Pausable]
* https://github.com/OpenZeppelin/cairo-contracts/blob/main/src/openzeppelin/token/erc20/presets/ERC20Upgradeable.cairo[ERC20Upgradeable]
* https://github.com/OpenZeppelin/cairo-contracts/blob/main/src/openzeppelin/token/erc20/presets/ERC20.cairo[ERC20]
* https://github.com/OpenZeppelin/cairo-contracts/blob/main/src/openzeppelin/token/erc721/presets/ERC721MintableBurnable.cairo[ERC721MintableBurnable]
* https://github.com/OpenZeppelin/cairo-contracts/blob/main/src/openzeppelin/token/erc721/presets/ERC721MintablePausable.cairo[ERC721MintablePausable]
* https://github.com/OpenZeppelin/cairo-contracts/blob/main/src/openzeppelin/token/erc721/enumerable/presets/ERC721EnumerableMintableBurnable.cairo[ERC721EnumerableMintableBurnable]
Copy link
Contributor

Choose a reason for hiding this comment

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

I would avoid links to main, they can/will break in the future. Not sure if there is a good alternative that won't become out of date too quickly though...

martriay and others added 2 commits August 3, 2022 19:13
@martriay martriay merged commit 298dce8 into main Aug 3, 2022
@martriay martriay deleted the integrate-docsite branch August 3, 2022 17:34
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.

Integrate docs into existing OZ docsite
3 participants