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

Fixes: Insert a Mermaid diagram in docs/contributing/core/package-dependencies.md #1715

Merged
merged 19 commits into from
Nov 26, 2024

Conversation

jensens
Copy link
Member

@jensens jensens commented Sep 26, 2024

Fixes #1683


📚 Documentation preview 📚: https://plone6--1715.org.readthedocs.build/

@stevepiercy
Copy link
Contributor

Unfortunately we cannot support Mermaid diagrams in plone-sphinx-theme. See:

I would love to support mermaid, but until that issue is resolved, I can't merge this PR. We can keep it open until then.

@jensens
Copy link
Member Author

jensens commented Sep 26, 2024

Unfortunately we cannot support Mermaid diagrams in plone-sphinx-theme

Yay.

@stevepiercy
Copy link
Contributor

@jensens let's revisit this. See #1777.

@stevepiercy
Copy link
Contributor

stevepiercy commented Nov 14, 2024

@jensens I went ahead and did a review by directly editing the file. I approve. Is this ready to merge? It's marked as Draft.

Check it out: https://plone6--1715.org.readthedocs.build/contributing/core/package-dependencies.html

@stevepiercy
Copy link
Contributor

@jensens I made a couple of enhancements to the Mermaid diagrams. Now they have alt, caption, and zoom attributes.

I don't understand the second diagram, but it's pretty.

@stevepiercy
Copy link
Contributor

@jensens would you please take a look at the pull request preview at https://plone6--1715.org.readthedocs.build/conceptual-guides/package-dependencies.html and let me know if this is ready to merge or you want to make any changes? I moved it into the Conceptual Guides from contributing to Plone core, since this is not really a how-to guide. Thank you!

@stevepiercy
Copy link
Contributor

@jensens can you please explain how you generated the mermaid diagrams?

I can update the docs accordingly. I would gratefully accept any notes you can create, anything that helps as reproduce what you did with these three diagrams. Maybe we can automate it. Thank you!

@jensens
Copy link
Member Author

jensens commented Nov 22, 2024

@jensens can you please explain how you generated the mermaid diagrams?

I can update the docs accordingly. I would gratefully accept any notes you can create, anything that helps as reproduce what you did with these three diagrams. Maybe we can automate it. Thank you!

I used https://www.mermaidchart.com/ to get a preview of the diagram I wrote and copied it over.

@stevepiercy
Copy link
Contributor

@jensens that's a nice tool.

Did you use any other sources of data to put into the diagrams, such as a requirements file or some other dependency generator?

I'm concerned that diagrams will not be updated with each release because no one knows how to maintain them.

I also notice that we need to increase contrast in dark mode for the diagram, as some lines do not show up.

@jensens
Copy link
Member Author

jensens commented Nov 25, 2024

Did you use any other sources of data to put into the diagrams, such as a requirements file or some other dependency generator?

This is out of my head. It is an architectural overview, not something one can generate. The level of abstraction is nowhere in our metadata.

The section "Packages in detail" could be generated - with some curation afterwards.

I'm concerned that diagrams will not be updated with each release because no one knows how to maintain them.

Indeed, we need to review them with each major release.

I also notice that we need to increase contrast in dark mode for the diagram, as some lines do not show up.

It seems to be an issue with the theme, as the background of the mermaid diagram changes to black.

@stevepiercy
Copy link
Contributor

For how to generate the diagrams and the section Packages in detail, I can ask @mauritsvanrees to add items to the release checklist. Any written notes for how to update these items would be great to have outside of your brain.

For the contrast of the Mermaid diagram on dark mode, I am reluctant to change the default background color of the pre selector, which is what all transparent SVG images use, because it would mess up the display and contrast of code blocks. The upstream themes PyData Sphinx Theme and Sphinx Book Themes have the resources to test usability and accessibility, but Plone Sphinx Theme is just me at the moment.

I would prefer to edit the SVG and add greater contrast as needed and test it, but then how do we collaborate on it going forward? As soon as you generate a new Mermaid diagram, any changes I put into it to fix the contrast would be obliterated.

@stevepiercy
Copy link
Contributor

@jensens oh, I'm an idiot. Duh, the source of the SVG as right there! I'll see if I can tweak it with some settings using that online tool.

@stevepiercy
Copy link
Contributor

@jensens I fiddled around with the source of the Mermaid diagram, and it now has some contrast, not sufficient for accessibility, but better than before, so I'll take it.

Is this ready to merge? I know it's not complete, but this is a vast improvement from where we are currently, and we can always iterate to improve it. I can also create new issues for anyone to pick up. Please let me know.

mauritsvanrees
mauritsvanrees previously approved these changes Nov 25, 2024
@stevepiercy
Copy link
Contributor

@mauritsvanrees where do you keep your release checklist? Let's add these to it, noting that the first two links are not yet live, but will be upon a merge of this PR.

@stevepiercy stevepiercy marked this pull request as ready for review November 25, 2024 23:19
@mauritsvanrees
Copy link
Member

The release checklist is in the coredev buildout. Each branch (6.0, 6.1) has its own slightly different list.

But the list of packages hardly ever changes. There are differences between 6.0 and 6.1 of course, but now that 6.1 is in beta, I don't expect any changes until 6.2 or 7. So adding this to the checklist may not be useful very often.

@stevepiercy
Copy link
Contributor

@mauritsvanrees pull request created at plone/buildout.coredev#968

I left out the Mermaid diagrams after all, now that I have a better grasp of their intent to describe a concept without overwhelming detail.

@stevepiercy stevepiercy merged commit 5332823 into 6.0 Nov 26, 2024
3 checks passed
@stevepiercy stevepiercy deleted the fix-1683-mermaid-deps branch November 26, 2024 08:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Insert a Mermaid diagram in conceptual-guides/package-dependencies.md
3 participants