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.

Sign up for GitHub

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

Jump to bottom

Add basic options and metadata #214

Merged
merged 12 commits into from
Aug 25, 2021
Merged

Add basic options and metadata #214

merged 12 commits into from
Aug 25, 2021

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

@kvid kvid mentioned this pull request Feb 11, 2021
Merged
@kvid kvid mentioned this pull request Feb 20, 2021
Merged
@kvid kvid force-pushed the issue158-options branch 2 times, most recently from fc47d4b to 75a9a75 Compare February 25, 2021 23:46
@kvid kvid mentioned this pull request Mar 19, 2021
Closed
@formatc1702 formatc1702 mentioned this pull request Mar 20, 2021
Merged
@formatc1702
Copy link
Collaborator

formatc1702 commented Mar 20, 2021 •
edited
Loading

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 •
edited
Loading

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.

@formatc1702 formatc1702 mentioned this pull request Mar 23, 2021
Draft
formatc1702
formatc1702 reviewed Mar 26, 2021
View reviewed changes
src/wireviz/wireviz.py Outdated Show resolved Hide resolved
@kvid kvid mentioned this pull request May 5, 2021
Open
@formatc1702 formatc1702 mentioned this pull request Aug 22, 2021
Open
kvid and others added 10 commits August 24, 2021 09:27
@kvid
Add basic metadata and options to Harness
5d6d60c 
Resolves the basic part of wireviz#158.
@kvid
Increase HTML readability by adding indents and linebreaks
2d6b1e3
@kvid
Apply options.color_mode where showing color as text
09243fb
@kvid
Show cable color in BOM similar to wires and connectors
66fdfc7
@kvid
Add fontname option
5aac2ba 
Use this option value both in the graph and as font family in HTML.
@kvid
Add bgcolor option
22f20e2 
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".
@kvid
Avoid transparent cable nodes
3223861 
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.
@kvid
Avoid a node size greater than the HTML label
84772fc 
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.
@kvid
Add bgcolor_node option
d5c6911 
It is default white unless set to a different color, or equal
bgcolor (as if transparent) if set to ~, null, Null, or NULL.
@kvid @formatc1702
Change Metadata to a dict as requested by the owner
15d7374 
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!

formatc1702
formatc1702 approved these changes Aug 25, 2021
View reviewed changes
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!

@formatc1702 formatc1702 merged commit 92354e6 into wireviz:dev Aug 25, 2021
@formatc1702 formatc1702 mentioned this pull request Aug 25, 2021
Closed
formatc1702 added a commit that referenced this pull request Aug 25, 2021
@formatc1702
Add #158, #214
9cbecbd
This was referenced Aug 25, 2021
Open
Merged
@kvid kvid mentioned this pull request Oct 23, 2021
Draft
formatc1702 added a commit that referenced this pull request Jun 7, 2023
@formatc1702
Add #158, #214
4c8722f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@formatc1702 formatc1702 formatc1702 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@kvid @formatc1702