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

Sprinkle some more demo on the docs #2139

Merged
merged 4 commits into from
Jan 9, 2023

Conversation

cartermp
Copy link
Contributor

@cartermp cartermp commented Jan 3, 2023

fixes #2137

There's several ways to skin this cat I guess, but I went with a shortcode shared across a few pages.

Preview: https://deploy-preview-2139--opentelemetry.netlify.app/docs/

image

image

@cartermp cartermp requested review from a team as code owners January 3, 2023 22:38
@cartersocha
Copy link
Contributor

Lgtm in general. One nit is to bold the demo title in the TOC like the other items. Ex. K8s operator. Thanks for doing this!

@cartermp
Copy link
Contributor Author

cartermp commented Jan 3, 2023

Fixed!

Copy link
Member

@julianocosta89 julianocosta89 left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

Copy link
Member

@mviitane mviitane left a comment

Choose a reason for hiding this comment

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

Looks very good! Added just few minor comments.

@@ -1,12 +1,8 @@
---
title: OpenTelemetry Demo
linkTitle: Demo
description:
The OpenTelemetry Demo is a microservice-based distributed system intended to illustrate the implementation of OpenTelemetry in a near real-world environment.
Copy link
Member

Choose a reason for hiding this comment

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

This same sentence is needed in 3 places. I don't know the docs structure, but I'm thinking if there's a way to have it only in one place and include it from there. It would be easier when changes are needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not aware of how to template these descriptions. I'm happy to also move it and bring it down into the text itself, but this also removes the description from google search results.

Copy link
Member

Choose a reason for hiding this comment

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

We can follow up on this, @chalin might have a suggestion/solution for this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi. The fact that this duplication exists is a bad smell IMHO. For a suggested alternative way of achieving this, see:

layouts/shortcodes/demo_description.md Outdated Show resolved Hide resolved
layouts/shortcodes/demo_description.md Outdated Show resolved Hide resolved
@svrnm
Copy link
Member

svrnm commented Jan 9, 2023

I like it!

Eventually I think we should remove one of the Demo links in the docs (either we have it beyond "Getting Started" or as top level item), but maybe we can track which one is used more and then later drop the other one?

@cartermp cartermp merged commit ff67422 into open-telemetry:main Jan 9, 2023
@cartermp cartermp deleted the cartermp/demo-sprinkle branch January 9, 2023 16:35
@cartermp
Copy link
Contributor Author

cartermp commented Jan 9, 2023

Yep, we can track via GA I believe.

@cartersocha
Copy link
Contributor

I would hesitate to contribute the full increase to the docs change as the new year really starts but there seems to be a noticeable increase in demo repo traffic from the website which is great to see.

Screenshot 2023-01-13 at 2 36 13 PM

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.

Add the OTel demo at a top level of docs
6 participants