Skip to content

Docs: graph edits to #25730#26160

Merged
alexfornuto merged 2 commits intopaul.gottschling/2023-05-05-mermaidfrom
afornuto/25730-suggest
May 17, 2023
Merged

Docs: graph edits to #25730#26160
alexfornuto merged 2 commits intopaul.gottschling/2023-05-05-mermaidfrom
afornuto/25730-suggest

Conversation

@alexfornuto
Copy link
Copy Markdown
Contributor

Styling suggestions for mermaid graphs in #25730.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This pushes the label so it's not obstructed by the lines.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is there a more elegant way to achieve this? If not, I think we should add a comment above this line indicating why we have all of the non-breaking spaces.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

There is, but only for left-right diagrams. See mermaid-js/mermaid#1209 for discussion on this issue. I'll add the comment as suggested.

@github-actions github-actions Bot requested review from r0mant, xinding33 and zmb3 May 12, 2023 18:19
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

At one point I had the idea to differentiate between Teleport Auth/Proxy and other Teleport services with colors. Happy to discuss if it's a good idea or not.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maybe it's just me/my monitor, but these colors look very similar to me! Should we just use one Teleport color?

Personally, I'm in favor of leaving all styling of our diagrams to an external stylesheet that we'll prepare later, but we can go with this approach in the meantime if you think it makes the graph clearer.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Agreed that it can be one color. I'm also in favor of an external style sheet and can remove the colors once it's created... but FWIW leaving one or two examples like this may give @roraback and team something to go on while building said stylesheets.

Comment thread docs/pages/get-started.mdx Outdated
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@ptgott I'm slightly concerned that this section of the graph describes a larger configuration than the page instructs.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I thought it would make sense to show the Linux server within the context of a full Teleport deployment, since ultimately the intended user of the guide would be expanding their setup from the initial VM to protect more infrastructure resources.

I also thought it would look a bit odd to only show the Linux server box.

What do you think?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think that the easy solve here is to add a note under the diagram explaining that it shows a scenario that extends beyond the contents of this page, and that adding additional resources is covered by the "Next steps" section.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm going to remove the diagram from the Linux Server guide so we can tackle that in a separate PR!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is there a more elegant way to achieve this? If not, I think we should add a comment above this line indicating why we have all of the non-breaking spaces.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maybe it's just me/my monitor, but these colors look very similar to me! Should we just use one Teleport color?

Personally, I'm in favor of leaving all styling of our diagrams to an external stylesheet that we'll prepare later, but we can go with this approach in the meantime if you think it makes the graph clearer.

Comment thread docs/pages/get-started.mdx Outdated
Comment thread docs/pages/get-started.mdx Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I thought it would make sense to show the Linux server within the context of a full Teleport deployment, since ultimately the intended user of the guide would be expanding their setup from the initial VM to protect more infrastructure resources.

I also thought it would look a bit odd to only show the Linux server box.

What do you think?

@alexfornuto alexfornuto requested a review from ptgott May 16, 2023 16:24
@alexfornuto alexfornuto force-pushed the afornuto/25730-suggest branch from b1504b7 to d1afac8 Compare May 17, 2023 15:37
@alexfornuto alexfornuto merged commit bc66abe into paul.gottschling/2023-05-05-mermaid May 17, 2023
@alexfornuto alexfornuto deleted the afornuto/25730-suggest branch May 17, 2023 15:37
ptgott added a commit that referenced this pull request May 22, 2023
* Add mermaid diagram to key pages

Add Mermaid diagrams to illustrate the architecture of a Teleport setup
in two guides where the audience may be encountering this architecture
for the first time:

- Try out Teleport on a Linux Server
- Deploying a High Availability Teleport Cluster

* Fix spelling

* Remove diagram from the Linux Server guide

We will handle this change separately, with more attention, since this
is a particularly high-traffic guide.

* Docs: graph edits to #25730 (#26160)

* graph edits

* add comment, rm secondary color

---------

Co-authored-by: Alex Fornuto <alex.fornuto@goteleport.com>
Co-authored-by: Steven Martin <steven@goteleport.com>
github-actions Bot pushed a commit that referenced this pull request May 22, 2023
* graph edits

* add comment, rm secondary color
github-actions Bot pushed a commit that referenced this pull request May 22, 2023
* graph edits

* add comment, rm secondary color
ptgott added a commit that referenced this pull request May 22, 2023
* Add mermaid diagram to key pages

Add Mermaid diagrams to illustrate the architecture of a Teleport setup
in two guides where the audience may be encountering this architecture
for the first time:

- Try out Teleport on a Linux Server
- Deploying a High Availability Teleport Cluster

* Fix spelling

* Remove diagram from the Linux Server guide

We will handle this change separately, with more attention, since this
is a particularly high-traffic guide.

* Docs: graph edits to #25730 (#26160)

* graph edits

* add comment, rm secondary color

---------

Co-authored-by: Alex Fornuto <alex.fornuto@goteleport.com>
ptgott added a commit that referenced this pull request May 22, 2023
* Add mermaid diagram to key pages

Add Mermaid diagrams to illustrate the architecture of a Teleport setup
in two guides where the audience may be encountering this architecture
for the first time:

- Try out Teleport on a Linux Server
- Deploying a High Availability Teleport Cluster

* Fix spelling

* Remove diagram from the Linux Server guide

We will handle this change separately, with more attention, since this
is a particularly high-traffic guide.

* Docs: graph edits to #25730 (#26160)

* graph edits

* add comment, rm secondary color

---------

Co-authored-by: Alex Fornuto <alex.fornuto@goteleport.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants