[Docs] Add sequence diagrams to fallback, retry, and rate limiter #1702
Conversation
| ### Happy path sequence diagram | ||
|
|
||
| ```mermaid | ||
| %%{init: {'theme':'dark'}}%% |
There was a problem hiding this comment.
This looks quite bad if you don't use the dark theme: #1701 (comment)
There was a problem hiding this comment.
On the mermaid documentation site the diagrams are dynamically switching between default and dark theme depending on the selected appearance mode. I haven't figured out yet how they do that so, I will spend some time to dig deeper.
There was a problem hiding this comment.
How about we just don't do anything and raise an issue in DocFx repo?
My thinking is that default behavior should handle this automatically.
There was a problem hiding this comment.
I would agree that it should just work.
If we can't make it look good for both light and dark, then we should use whichever variant looks less worse.
How does the default/light diagram look in dark mode?
There was a problem hiding this comment.
The defaults for white theme:
Dark theme:
There was a problem hiding this comment.
I've found how they do it. :)
const renderChart = async () => {
const hasDarkClass = document.documentElement.classList.contains('dark');
const mermaidConfig = {
...
theme: hasDarkClass ? 'dark' : 'default',
};
...
}I don't think we can do similar via the docfx config :(
There was a problem hiding this comment.
Is there a way to add a custom module/script to apply a global config or something when the page is being rendered in the client? Docs
If not, would be good to create an issue in the docfx repo if there isn't one already requesting this feature.
There was a problem hiding this comment.
@martintmk, @martincostello I've filed an issue on docfx: dotnet/docfx#9306.
There was a problem hiding this comment.
Shall we add a note to the diagrams sections something like:
Please prefer dark appearance mode for better legibility ... it is a temporarily solution
?
There was a problem hiding this comment.
Let's avoid a comment - otherwise if we have lots of diagrams I think it's just going to duplicate all over the place.
Codecov ReportAll modified lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1702 +/- ##
=======================================
Coverage 84.65% 84.65%
=======================================
Files 306 306
Lines 6829 6829
Branches 1046 1046
=======================================
Hits 5781 5781
Misses 839 839
Partials 209 209
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
@martincostello The Can we temporary make it optional until Roslyn team figures what's happening? |
I don't have the powers to change that, but yeah I noticed it's been worse than usual which is why I finally logged the issue 😅 |
Maybe disable all analyzers when |
Do you want to do a PR to turn them off? The trouble is, CodeQL might be implemented as analyzers for C#, then basically making the whole thing run pointlessly. |
Pull Request
The issue or feature being addressed
Details on the issue fix or feature implementation
Confirm the following