-
Notifications
You must be signed in to change notification settings - Fork 792
<refactor>(artifacts): Broke long page into subpages #1193
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
base: main
Are you sure you want to change the base?
Conversation
…ation via the nav bar
|
👋 Thanks for your contribution! |
koverholt
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR and improvement!
This is a solid refactor that improves the usability of the artifacts docs. Breaking the long single page into focused subpages lines up with how we've been restructuring other docs sections to make it easier for developers (and AI coding tools) to find what they need in a more modular fashion.
I left some comments related to typos, newlines at end of files, language tags, broken anchor links, a couple of code bugs, and a nav suggestion.
Would also like a second review from @joefernandez on this reorganization.
| @@ -0,0 +1,123 @@ | |||
| ## Available Implementations | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
docs/artifacts/core-concepts.md
Outdated
| @@ -0,0 +1,252 @@ | |||
| ## Core Concepts | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change these to H1 to avoid duplicate H1 (page title) and H2 headings.
docs/artifacts/best-practices.md
Outdated
| @@ -0,0 +1,18 @@ | |||
| ## Best Practices | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change these to H1 to avoid duplicate H1 (page title) and H2 headings.
| @@ -0,0 +1,538 @@ | |||
| ## Interacting with Artifacts (via Context Objects) | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change these to H1 to avoid duplicate H1 (page title) and H2 headings.
mkdocs.yml
Outdated
| - Artifacts: | ||
| - artifacts/index.md | ||
| - Core Concepts: artifacts/core-concepts.md | ||
| - Inteacting with Artifacts: artifacts/interacting-with-artifacts.md |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo in "Interacting".
| // Example: Configuring the Runner with an Artifact Service | ||
| const myAgent = new LlmAgent({name: "artifact_user_agent", model: "gemini-2.5-flash"}); | ||
| const artifactService = new InMemoryArtifactService(); // Choose an implementation | ||
| const sessionService = new InMemoryArtifactService(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not the fault of this PR, but this should be:
const sessionService = new InMemorySessionService();| - artifacts/index.md | ||
| - Core Concepts: artifacts/core-concepts.md | ||
| - Inteacting with Artifacts: artifacts/interacting-with-artifacts.md | ||
| - Available Implementations: artifacts/available-implementations.md |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggest putting "Available Implementations" above "Interacting with Artifacts" in the nav, since developers can read through this and select their backend before getting into the interaction patterns.
| @@ -0,0 +1,123 @@ | |||
| ## Available Implementations | |||
|
|
|||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggest adding language tags like on the index page, on all sub-pages in this PR except for best practices.
| @@ -0,0 +1,252 @@ | |||
| ## Core Concepts | |||
|
|
|||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggest adding language tags like on the index page, on all sub-pages in this PR except for best practices.
| @@ -0,0 +1,538 @@ | |||
| ## Interacting with Artifacts (via Context Objects) | |||
|
|
|||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggest adding language tags like on the index page, on all sub-pages in this PR except for best practices.
| }); | ||
| // If no artifactService is configured, calling artifact methods on context objects will throw an error. | ||
| ``` | ||
| In Java, if an `ArtifactService` instance is not available (e.g., `null`) when artifact operations are attempted, it would typically result in a `NullPointerException` or a custom error, depending on how your application is structured. Robust applications often use dependency injection frameworks to manage service lifecycles and ensure availability. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Happened before this PR, but this line should be moved to the Java tab.
|
Thanks for this contribution, @tommywagz ! Unfortunately, just splitting up this page into multiple pages is not a useful information refactoring. The net effect of this change is that it makes getting to the information harder for a developer reader, because now the existing information is split across multiple pages. You are making a mechanical change that doesn't make the information any easier to use. So let's talk about how to refactor this page to make it more useable: The core information design problem with this page is that is mixes conceptual content and practical application content in a confusing way. For example, it's difficult to see the basics of implementing artifacts in an agent because the code samples are buried in a section called "Core Concepts" which isn't a likely place for folks to look for basic getting started code. My general requirement for refactoring this page is to split the content into 2 pages (not 5 pages): Page 1. Practical implementation of artifacts in agents with a brief but useful introduction to the problems this feature solves and the bare minimum of conceptual information This split requires refactoring the content more than just splitting out the sections. You need to break apart the sections themselves so that the practical code examples are clearly presented and findable (clear, action-oriented headings like "Get started" and "Read data from artifacts", and you need to preserve and re-organize the conceptual content so that presents information logically, and helps more advanced users navigate advanced use of the feature. I'm setting a high bar for the refactoring, but it's what it actually needed to really improve the usefulness of content. I understand if this is not what you though you were signing up for. :-) Regardless, thanks for showing an interest in improving the ADK docs. If this refactoring seems like too much, please consider other, less involved contributions. Thanks! \Joe |

Broke the Artifacts page into sub-pages consistent with previous subheadings:
... First commit here - just testing the waters