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

Adding Images to Orchestration Docs #1238

Merged
merged 2 commits into from
Oct 16, 2024
Merged

Adding Images to Orchestration Docs #1238

merged 2 commits into from
Oct 16, 2024

Conversation

amessbee
Copy link
Contributor

@amessbee amessbee commented Oct 9, 2024

Here are the changes:

  1. Adding an orchestration contract architecture graphic.
  2. Adding a graphic with potential connections between chains established through Agoric Orchestration.
  3. Updating sequence diagrams to use mermaid code directly in markdown.
  4. Removing unused assets.

Copy link

cloudflare-workers-and-pages bot commented Oct 9, 2024

Deploying documentation with  Cloudflare Pages  Cloudflare Pages

Latest commit: a15322c
Status: ✅  Deploy successful!
Preview URL: https://a93bea8a.documentation-7tp.pages.dev
Branch Preview URL: https://ms-add-orch-images.documentation-7tp.pages.dev

View logs

@@ -10,6 +10,10 @@ The Agoric Orchestration API sits on top of Agoric’s novel VM that provides th

Agoric’s Orchestration APIs simplify controlling accounts on remote chains, moving assets, and using capabilities on any chain the API reaches.

<br/>
<img src="./assets/orch_arch.svg" width="125%" />
<br/>
Copy link
Member

Choose a reason for hiding this comment

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

Nice figure. I'd like to see it right after the 1st paragraph (ending "novel cross-chain-focused products.")

Copy link
Member

Choose a reason for hiding this comment

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

Also, how do we maintain it? Is this .svg file the editable source form? What tool did you use to create it?

Copy link
Contributor Author

@amessbee amessbee Oct 11, 2024

Choose a reason for hiding this comment

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

@dckc I created the original image using latex in ipe tool, and yes this was SVG.

As per offline comments by @toliaqat I have updated the image with a new version, and moved it to how-orchestration-works page. This image uses the original source image from Agoric Orchestration Docs document and was created using Canva online editor for which SVG is not available.

Copy link
Member

Choose a reason for hiding this comment

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

Canva? I found it in some Google slides. Here's an SVG export:

orch-arch

Copy link
Contributor

Choose a reason for hiding this comment

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

@dckc can you update the text in "User Story" box as per the image mudassir uploaded, and reupload the SVG?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think svg vs png is a debate that should delay improving docs. We already have both kinds of images in the repo and they solve the problem at hand. Optimizing the format should come when we have learned enough after many iterations and have nothing else to improve.

Copy link
Member

Choose a reason for hiding this comment

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

... We already have both kinds of images in the repo and they solve the problem at hand.

Yes; we use .png for screenshots and, until now, we used .svg for diagrams.

Optimizing the format should come when we have learned enough after many iterations and have nothing else to improve.

So only after many iterations we should consider accessibility? That's not a position I'm prepared to support.

https://www.ohchr.org/en/instruments-mechanisms/instruments/convention-rights-persons-disabilities#article-9

@@ -15,6 +15,10 @@ To achieve these goals, several foundational technologies and protocols have
been developed, with the [Inter-Blockchain Communication (IBC) protocol](https://ibcprotocol.org/) being
one of the most prominent.

<br/>
<img src="./assets/orch_contract_arch.png" width="100%" />
Copy link
Member

Choose a reason for hiding this comment

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

any particular reason to switch from .svg to .png? .svg is more accessible - the text is searchable and selectable

Copy link
Member

Choose a reason for hiding this comment

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

oic

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 think free version of Canva does not let you download SVG.

Back to SVG again for that image by converting the source slide you linked. There is another image on the landing page now (in PNG for now). I would probably tinkering a bit more, and perhaps add one more image too.

Copy link
Contributor

Choose a reason for hiding this comment

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

the text is searchable and selectable

@dckc how is the text searchable and selectable in the SVG image? I cannot do that in your uploaded SVG. I could not find DEX in the source of your image as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

@toliaqat toliaqat left a comment

Choose a reason for hiding this comment

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

@amessbee please remove images from the PR that are not needed anymore. For example: main/guides/orchestration/assets/orch_arch.svg

Copy link

github-actions bot commented Oct 14, 2024

Cloudflare deployment logs are available here

@amessbee
Copy link
Contributor Author

amessbee commented Oct 14, 2024

@dckc @mujahidkay @toliaqat I am adding a vite plugin for mermaid that renders diagrams from within markdown and seems to work fine. This will save us from keeping both .mmd source file and the .svg graphic file and we can directly add the mermaid code to the markdown. I have updated the sequence diagram on Orchestration landing page - never mind rest of the mess on that page for now. I will update docs wherever we are using mermaid code unless there is a reason not to.

@amessbee amessbee force-pushed the ms/add-orch-Images branch 3 times, most recently from 026a281 to 4fae29d Compare October 14, 2024 12:54
@amessbee amessbee marked this pull request as ready for review October 14, 2024 12:54
@@ -1,8 +1,9 @@
import { defineConfig } from 'vitepress';
Copy link
Member

Choose a reason for hiding this comment

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

this import is not needed anymore

@@ -10,6 +10,10 @@ The Agoric Orchestration API sits on top of Agoric’s novel VM that provides th

Agoric’s Orchestration APIs simplify controlling accounts on remote chains, moving assets, and using capabilities on any chain the API reaches.

<br/>
<img src="./assets/chains4.png" width="100%" />
Copy link
Member

Choose a reason for hiding this comment

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

nit: appropriate filename chains.png should be fine

package.json Outdated
@@ -67,10 +67,12 @@
"eslint-plugin-prettier": "^5.2.1",
"glob": "7.1.7",
"import-meta-resolve": "^2.2.2",
"mermaid": "^11.3.0",
Copy link
Member

Choose a reason for hiding this comment

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

we also want to probe agoric-sdk CI against this branch. This is to avoid any underlying package used by these new packages to break it (it's broken right now so we may have to wait for #1240 to merge and then rebase)

@toliaqat
Copy link
Contributor

@amessbee I really like the mermaid code and rendering package—it’s great that it allows users to select and search text on the page. Thanks for adding that! However, I would have preferred it as a separate PR so we could review and merge the graphic and mermaid PRs independently.

@amessbee
Copy link
Contributor Author

@amessbee I really like the mermaid code and rendering package—it’s great that it allows users to select and search text on the page. Thanks for adding that! However, I would have preferred it as a separate PR so we could review and merge the graphic and mermaid PRs independently.

Of course. Created a separate branch - here is PR #1243

Copy link
Member

@mujahidkay mujahidkay left a comment

Choose a reason for hiding this comment

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

Good addition to the docs!

Kindly update branch and re-verify before merging.

@amessbee amessbee force-pushed the ms/add-orch-Images branch 2 times, most recently from 1097ba5 to 10f5a0c Compare October 16, 2024 09:56
@amessbee amessbee merged commit 6e2685c into main Oct 16, 2024
6 checks passed
@amessbee amessbee deleted the ms/add-orch-Images branch October 16, 2024 13:25
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.

4 participants