Skip to content

Conversation

emnul
Copy link
Contributor

@emnul emnul commented Sep 25, 2025

No description provided.

Copy link

netlify bot commented Sep 25, 2025

Deploy Preview for openzeppelin-docs-v2 ready!

Name Link
🔨 Latest commit 5de8da3
🔍 Latest deploy log https://app.netlify.com/projects/openzeppelin-docs-v2/deploys/68db04aa67c61d000816ac66
😎 Deploy Preview https://deploy-preview-43--openzeppelin-docs-v2.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

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.

Nice work on the improvements @emnul! I left a few comments

Comment on lines +96 to 102
### Witnesses [toc] [#AccessControl-Witnesses]
### Witnesses [!toc] [#AccessControl-Witnesses]
None.
### Circuits [toc] [#AccessControl-Circuits]
### Circuits [!toc] [#AccessControl-Circuits]
Copy link
Collaborator

Choose a reason for hiding this comment

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

I omitted the Ledger, Witnesses, and Circuits grouping names from the ToC because this breaks the semantic hierarchy

<h2>Core</h2>
<h3>AccessControl</h3>
<h3>Ledger</h3>  // Is within AccessControl but is displayed as adjacent
<h4>fooSig</h4>

We can remove core and promote AccessControl as an h2 header like in the Ownable API so the hierarchy would be:

<h2>AccessControl</h2>
<h3>Ledger</h3>
<h4>fooSig</h4>

Copy link
Collaborator

Choose a reason for hiding this comment

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

If we like this approach, the same applies to the other modules with a Core header

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 it'd be even better if we had a dropdown for these groupings in the ToC. Outside the scope of this PR though

@emnul emnul marked this pull request as ready for review September 30, 2025 13:52
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.

Very nice!

@emnul emnul merged commit 78d2f53 into main Sep 30, 2025
6 checks passed
@emnul emnul deleted the update-compact-docs branch October 10, 2025 17:27
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.

2 participants