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

Add basic options and metadata #214

Merged
merged 12 commits into from
Aug 25, 2021
Merged

Conversation

kvid
Copy link
Collaborator

@kvid kvid commented Feb 9, 2021

  • Add metadata attributes: title, description, notes
  • Add option attributes: color_mode, mini_bom_mode
  • Use metadata values and a few more indents and linebreaks in the HTML output
  • Translate color text according to options.color_mode a few more places
  • This solves the initial part of [feature] Accept optional (and overridable) harness attribute(s) #158

@formatc1702
Copy link
Collaborator

formatc1702 commented Mar 20, 2021

Does it make sense to keep the metadata section more general, as a dictionary to be filled with user-defined key/value pairs instead of limiting it to certain fixed parameters?
Certain keywords could have effects in the GraphViz output (title maybe?), whereas others could be accessed from HTML when outputting a full technical drawing and allow for expansion with parameters that users will think of in the future for use with their own templates.

@kvid
Copy link
Collaborator Author

kvid commented Mar 21, 2021

Does it make sense to keep the metadata section more general, as a dictionary to be filled with user-defined key/value pairs instead of limiting it to certain fixed parameters?

If you by this mean replacing the Metadata dataclass with a dict, then that is quite easy to do. The drawback is more complex syntax when using the values in the code.

Certain keywords could have effects in the GraphViz output (title maybe?),

The currently suggested code in this PR only use metadata for generating HTML.

whereas others could be accessed from HTML when outputting a full technical drawing and allow for expansion with parameters that users will think of in the future for use with their own templates.

@formatc1702
Copy link
Collaborator

formatc1702 commented Mar 21, 2021

Does it make sense to keep the metadata section more general, as a dictionary to be filled with user-defined key/value pairs instead of limiting it to certain fixed parameters?

If you by this mean replacing the Metadata dataclass with a dict, then that is quite easy to do. The drawback is more complex syntax when using the values in the code.

For now, I would indeed request to replace the dataclass with a dict, to stay flexible, and try to improve syntax later. I don't see the more complex syntax as a big problem since it is used in a limited number of places.

A different, but IMHO uglier option, is to keep a regular Metadata class that includes a custom_fields dict within it, but this nesting would have to be reflected in the YAML input, making the user experience worse.

A wilder idea, which I would like to explore in a separate PR, is implementing dot notation for a dict. By creating a custom class like that, you could force it to have the standard attributes (title, description, notes in your suggested code) and auto-fill them with default values in the class' __init__() if the YAML input does not provide them, and at the same time, leave it flexible and open to user-defined attributes they might need in their drawing template. There may be unexpected drawbacks and complications, so we should first close this PR with the simplest and most flexible implementation: a generic dict.

Certain keywords could have effects in the GraphViz output (title maybe?),

The currently suggested code in this PR only use metadata for generating HTML.

Just leaving this as an idea for the future.

src/wireviz/wireviz.py Outdated Show resolved Hide resolved
kvid and others added 10 commits August 24, 2021 09:27
Use this option value both in the graph and as font family in HTML.
The same color value is used for html.body.style.background-color
and gv.graph.bgcolor to make the diagram fit seamlessly in the HTML
output. "bgcolor" is chosen as option name to avoid the dash in the
CSS name "background-color".
Bug: Node attribute fillcolor is ignored unless style contains filled.
When cable nodes get an empty style (unless bundle), the node background
color become equal to the graph background color as it was transparent.

Fix: Make the style contain filled.
Bug: When the HTML label is narrow (e.g. connectors in tutorial01),
then the white node background is wider, and when the HTML label is
low (e.g. ferrules in tutorial05 and totorial06), then the white
node background is taller. Such errors are easily seen with a
non-white bgcolor.

Fix: Set node width, height, and margin to zero to let the actual
size of the node be entirely determined by the label.
It is default white unless set to a different color, or equal
bgcolor (as if transparent) if set to ~, null, Null, or NULL.
And a few other changes requested in the same review.

Co-authored-by: Daniel Rojas <github@danielrojas.net>
@formatc1702
Copy link
Collaborator

I have tried out the code, and it works well. I also have no issues with the code style itself.

Would you mind adding the required documentation to syntax.md? It's the only thing missing for merging.
Thanks!

Copy link
Collaborator

@formatc1702 formatc1702 left a comment

Choose a reason for hiding this comment

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

Thanks for adding the syntax description!

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