-
Notifications
You must be signed in to change notification settings - Fork 39
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
Conversation
d918108
to
f2e94df
Compare
Deploying documentation with Cloudflare Pages
|
@@ -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/> |
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.
Nice figure. I'd like to see it right after the 1st paragraph (ending "novel cross-chain-focused products.")
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.
Also, how do we maintain it? Is this .svg file the editable source form? What tool did you use to create it?
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.
@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.
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.
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.
@dckc can you update the text in "User Story" box as per the image mudassir uploaded, and reupload the SVG?
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.
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.
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.
... 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.
@@ -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%" /> |
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.
any particular reason to switch from .svg to .png? .svg is more accessible - the text is searchable and selectable
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.
oic
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.
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.
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.
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.
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.
I thought I already fixed that?
https://8abcef3f.documentation-7tp.pages.dev/guides/orchestration/how-orch-works.html
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.
@amessbee please remove images from the PR that are not needed anymore. For example: main/guides/orchestration/assets/orch_arch.svg
Cloudflare deployment logs are available here |
@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 |
026a281
to
4fae29d
Compare
main/.vitepress/config.mjs
Outdated
@@ -1,8 +1,9 @@ | |||
import { defineConfig } from 'vitepress'; |
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.
this import is not needed anymore
main/guides/orchestration/index.md
Outdated
@@ -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%" /> |
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.
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", |
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.
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)
4fae29d
to
729b802
Compare
@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 |
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.
Good addition to the docs!
Kindly update branch and re-verify before merging.
1097ba5
to
10f5a0c
Compare
10f5a0c
to
a15322c
Compare
Here are the changes: