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

Added section images #65

Merged
merged 4 commits into from
Jun 8, 2020
Merged

Added section images #65

merged 4 commits into from
Jun 8, 2020

Conversation

jspaaks
Copy link

@jspaaks jspaaks commented Jun 5, 2020

To separate the content a bit better, I added section images. Hope you like it!

PS
When making these changes, I noticed that perhaps it's good to

  1. put the Swagger web service before the Flask app
  2. make the first part of the webassembly section result in a service instead of a HTML page

@jspaaks jspaaks mentioned this pull request Jun 5, 2020
@sverhoeven sverhoeven mentioned this pull request Jun 5, 2020
Copy link
Member

@sverhoeven sverhoeven left a comment

Choose a reason for hiding this comment

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

I like the flow diagram style.

The text between the 2 arrows is not consistent.

For JS you use emscripten in image, for Python we should add pybind11 in image.

images/flask.svg Outdated
x="1267.2537"
y="102.80756" /></flowRegion><flowPara
id="flowPara1006-2"
style="font-style:normal;font-variant:normal;font-weight:normal;font-stretch:normal;font-size:53.33333588px;font-family:'Roboto Condensed';-inkscape-font-specification:'Roboto Condensed, Normal';font-variant-ligatures:normal;font-variant-caps:normal;font-variant-numeric:normal;font-feature-settings:normal;text-align:start;writing-mode:lr-tb;text-anchor:start;fill:#333333">two-way C++ to Python bindings</flowPara></flowRoot> <flowRoot
Copy link
Member

Choose a reason for hiding this comment

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

Could you remove two way we are just calling C++ from Python and we are not calling Python from C++.

Suggested change
style="font-style:normal;font-variant:normal;font-weight:normal;font-stretch:normal;font-size:53.33333588px;font-family:'Roboto Condensed';-inkscape-font-specification:'Roboto Condensed, Normal';font-variant-ligatures:normal;font-variant-caps:normal;font-variant-numeric:normal;font-feature-settings:normal;text-align:start;writing-mode:lr-tb;text-anchor:start;fill:#333333">two-way C++ to Python bindings</flowPara></flowRoot> <flowRoot
style="font-style:normal;font-variant:normal;font-weight:normal;font-stretch:normal;font-size:53.33333588px;font-family:'Roboto Condensed';-inkscape-font-specification:'Roboto Condensed, Normal';font-variant-ligatures:normal;font-variant-caps:normal;font-variant-numeric:normal;font-feature-settings:normal;text-align:start;writing-mode:lr-tb;text-anchor:start;fill:#333333">C++ to Python bindings</flowPara></flowRoot> <flowRoot

Same goes for other occurences

Copy link
Author

@jspaaks jspaaks Jun 8, 2020

Choose a reason for hiding this comment

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

'two way" removed in next commit

images/react.svg Outdated
width="932.27899"
id="rect1004-7" /></flowRegion><flowPara
style="font-style:normal;font-variant:normal;font-weight:normal;font-stretch:normal;font-size:53.33333588px;font-family:'Roboto Condensed';-inkscape-font-specification:'Roboto Condensed, Normal';font-variant-ligatures:normal;font-variant-caps:normal;font-variant-numeric:normal;font-feature-settings:normal;text-align:start;writing-mode:lr-tb;text-anchor:start;fill:#333333"
id="flowPara1006-5">Emscriptem</flowPara></flowRoot> <flowRoot
Copy link
Member

Choose a reason for hiding this comment

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

Typo

Suggested change
id="flowPara1006-5">Emscriptem</flowPara></flowRoot> <flowRoot
id="flowPara1006-5">Emscripten</flowPara></flowRoot> <flowRoot

Copy link
Author

Choose a reason for hiding this comment

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

typo corrected in next commit

images/wasm.svg Outdated
width="932.27899"
id="rect1004-7" /></flowRegion><flowPara
style="font-style:normal;font-variant:normal;font-weight:normal;font-stretch:normal;font-size:53.33333588px;font-family:'Roboto Condensed';-inkscape-font-specification:'Roboto Condensed, Normal';font-variant-ligatures:normal;font-variant-caps:normal;font-variant-numeric:normal;font-feature-settings:normal;text-align:start;writing-mode:lr-tb;text-anchor:start;fill:#333333"
id="flowPara1006-5">Emscriptem</flowPara></flowRoot> </g>
Copy link
Member

Choose a reason for hiding this comment

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

Typo

Suggested change
id="flowPara1006-5">Emscriptem</flowPara></flowRoot> </g>
id="flowPara1006-5">Emscripten</flowPara></flowRoot> </g>

Copy link
Author

Choose a reason for hiding this comment

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

typo corrected in next commit

@jspaaks
Copy link
Author

jspaaks commented Jun 8, 2020

for Python we should add pybind11 in image.

I thought "Python bindings" was a bit more generic, therefore I left it like this. Let me know if you'd still like to see it changed.

@sverhoeven
Copy link
Member

for Python we should add pybind11 in image.

I thought "Python bindings" was a bit more generic, therefore I left it like this. Let me know if you'd still like to see it changed.

For consistencies sake we should have the flavor of bindings and the bindings library in both JS and Python diagrams. Or just have the flavor of bindings.

Copy link
Member

@sverhoeven sverhoeven left a comment

Choose a reason for hiding this comment

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

I think you don't need the png you can use the svg directly in Markdown

To distracting and not interactive enough
@jspaaks
Copy link
Author

jspaaks commented Jun 8, 2020

I think it's OK to sacrifice some consistency across the document if that makes an individual section image easier to understand. The purpose of the images is not to accurately capture all the details that are discussed in the corresponding text; rather, they provide an anchor for readers to recognize. In that role, I thought that "C++ to Python bindings" will be recognized by more readers than "pybind11".

@sonarqubecloud
Copy link

sonarqubecloud bot commented Jun 8, 2020

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@jspaaks
Copy link
Author

jspaaks commented Jun 8, 2020

On svg v png: On GitHub everything works ok, but at least my editor (Atom) struggles with rendering order of layers, e.g. text on top of areas. png is a bit more rebust in that sense.

@jspaaks jspaaks requested a review from sverhoeven June 8, 2020 14:23
Copy link
Member

@sverhoeven sverhoeven left a comment

Choose a reason for hiding this comment

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

OK, let's keep using png for editor support. I noticed with a dark theme in VS code the center text was unreadable aswell.

Let's also keep the text as is, after the reshuffling in #66, we will need to add and maybe update the section images.

@jspaaks
Copy link
Author

jspaaks commented Jun 8, 2020

dark theme in VS code

Same in atom. I'll add a white bg in the next round of edits after #66

@jspaaks jspaaks merged commit 39a767c into master Jun 8, 2020
@jspaaks jspaaks deleted the section-images branch June 8, 2020 16:43
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.

2 participants