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

[Docs] Add sequence diagrams to timeout strategy #1699

Conversation

peter-csala
Copy link
Contributor

@peter-csala peter-csala commented Oct 16, 2023

Pull Request

The issue or feature being addressed

  • When I was working on the Polly-Samples migration I've added several mermaid diagrams (for example: 1, 2 to visualize the relationships).
  • I thought it might make sense to further enhance the Polly documentation by adding sequence diagrams to it

Details on the issue fix or feature implementation

  • Added a happy path and unhappy path sequence diagram to the timeout strategy

Preview

Confirm the following

  • I started this PR by branching from the head of the default branch
  • I have targeted the PR to merge into the default branch
  • I have included unit tests for the issue/feature
  • I have successfully run a local build

Sorry, something went wrong.

@martintmk
Copy link
Contributor

Two notes:

  • Please check whether this is supported by DocFx since that one is used for our docs: https://www.pollydocs.org/strategies/timeout.html
  • I would move these under the Defaults section under Diagram or Diagram flow since these diagrams are for more advanced readers and w don't want to obscure the usage samples.

@codecov
Copy link

codecov bot commented Oct 16, 2023

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (862690e) 84.63% compared to head (7c795eb) 84.63%.
Report is 1 commits behind head on main.

❗ Current head 7c795eb differs from pull request most recent head 1b224a1. Consider uploading reports for the commit 1b224a1 to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1699   +/-   ##
=======================================
  Coverage   84.63%   84.63%           
=======================================
  Files         306      306           
  Lines        6819     6819           
  Branches     1045     1045           
=======================================
  Hits         5771     5771           
  Misses        839      839           
  Partials      209      209           
Flag Coverage Δ
linux 84.63% <ø> (?)
macos 84.63% <ø> (ø)
windows 84.63% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@martincostello
Copy link
Member

I'm not sure whether the pollydocs.org will do the same dynamic rendering as github.

Yeah, that was my first thought too.

@peter-csala peter-csala changed the title Add sequence diagrams to timeout strategy [Docs] Add sequence diagrams to timeout strategy Oct 16, 2023
@peter-csala peter-csala force-pushed the add-sequence-diagrams-to-timeout-strategy branch from 0951463 to 7c795eb Compare October 16, 2023 12:38
@peter-csala
Copy link
Contributor Author

* Please check whether this is supported by DocFx since that one is used for our docs: https://www.pollydocs.org/strategies/timeout.html

I've found these comments:

and this plugin: https://github.com/dpvreony/docfx-mermaidjs

* I would move these under the `Defaults` section under `Diagram` or `Diagram flow` since these diagrams are for more advanced readers and w don't want to obscure the usage samples.
  • Done

@martintmk
Copy link
Contributor

* Please check whether this is supported by DocFx since that one is used for our docs: https://www.pollydocs.org/strategies/timeout.html

I've found these comments:

and this plugin: https://github.com/dpvreony/docfx-mermaidjs

* I would move these under the `Defaults` section under `Diagram` or `Diagram flow` since these diagrams are for more advanced readers and w don't want to obscure the usage samples.
  • Done

Maybe just include these as images under docs/media ?

@martincostello
Copy link
Member

Maybe just include these as images under docs/media ?

That's probably the least friction way. We can still leave the diagrams in as HTML comments to make regenerating them easy.

@peter-csala
Copy link
Contributor Author

peter-csala commented Oct 16, 2023

Maybe just include these as images under docs/media ?

On pollydocs you can switch back and forth between light and dark themes.

If we could add mermaid support then the diagrams would align with the selected theme as well.

Screenshot 2023-10-16 at 14 54 29

If we stick with the image then which theme do we prefer?

@martincostello
Copy link
Member

I would go with "normal" black on white if we don't go with a plugin.

@martincostello martincostello enabled auto-merge (squash) October 16, 2023 14:42
@martincostello martincostello merged commit b169f88 into App-vNext:main Oct 16, 2023
@peter-csala
Copy link
Contributor Author

@martincostello , @martintmk Do you have any idea why the pictures are not shown on pollydocs?

@martincostello
Copy link
Member

My guess would be the paths are slightly different when the docfx website is built.

I suggest running it locally and seeing what path it outputs and then tweaking the file to have the right path for pollydocs even if it breaks GitHub.

@peter-csala peter-csala deleted the add-sequence-diagrams-to-timeout-strategy branch October 31, 2023 16:27
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.

None yet

3 participants