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 bgcolor attribute to connectors and cables #219

Merged
merged 7 commits into from
Sep 28, 2021

Conversation

kvid
Copy link
Collaborator

@kvid kvid commented Feb 16, 2021

  • This solves [feature] set Background color in Connector and Wire Box #210 completely by supporting bgcolor of both the node title
    and the whole node independently using separate attributes (bgcolor_title and bgcolor).
  • Supporting bgcolor of an image to enable adapting the table cell background to the image background.
  • To make it easier choosing background colors, I suggest adding more light colors into _color_hex. (hex input is now possible)
  • I suggest merging both Simplify BOM code #197 and Add basic options and metadata #214 before rebasing this PR on top, and then possibly add an extra commit here before it might be ready. (done)

Example usage together with other color attributes:
issue210

Click to expand YAML input
connectors:
  X1: &conn
    pinlabels: [+5V, GND]
    bgcolor: IV  # Node background color
  X2:
    <<: *conn
    bgcolor_title: YE  # Title row background color
    color: BG  # Connector housing color
    pincolors: [RD]  # Connector pin coloring
cables:
  W1:
    colors: [RD, BK]  # Wire colors
    bgcolor_title: PK  # Title row background color
    bgcolor: LB  # Node background color
    color: GY  # Cable jacket color
    gauge: 24 AWG
    show_equiv: True
connections:
  - - X1: [1-2]
    - W1: [1-2]
    - X2: [1-2]

@formatc1702
Copy link
Collaborator

Thanks for all these PRs and other submissions. I am keeping an eye on them but need to find some quiet time to properly try them out and integrate them, I cannot promise anything but it's definitely on my mind. I appreciate the work you are putting into the project!

@designer2k2
Copy link

Thanks a lot for this! I tested it now locally (after figuring out how to use the github desktop app) and it works!

I agree that more light colors would help, or even a way do use hex colors directly.

For me, i stay now on this until its merged 😀

kvid added a commit to kvid/WireViz that referenced this pull request Mar 18, 2021
This was requested by designer2k2 in wireviz#219 for bgcolor usage.
It has also been discussed in wireviz#135.

The input validation is more detailed to help the user identifying
and locating invalid values. The wire color padding is now done on
the output to cover different input alternatives.
@designer2k2
Copy link

wow, thanks for adding the hex color option!
I tested it and it works. 👍

@formatc1702
Copy link
Collaborator

formatc1702 commented Mar 20, 2021

Awesome stuff.

Would you mind adding the relevant info to the syntax documentation in a new PR, branched off and to be merged back into the feature/documentation branch? Alternatively, wait until the question in #181 is resolved and eventually submit the updated docs as part of this PR?

I'm sure a lot of people will appreciate this new feature.

@kvid kvid mentioned this pull request Mar 21, 2021
kvid added a commit to kvid/WireViz that referenced this pull request Mar 29, 2021
This was requested by designer2k2 in wireviz#219 for bgcolor usage.
It has also been discussed in wireviz#135.

The input validation is more detailed to help the user identifying
and locating invalid values. The wire color padding is now done on
the output to cover different input alternatives.
kvid added a commit to kvid/WireViz that referenced this pull request Aug 23, 2021
This was requested by designer2k2 in wireviz#219 for bgcolor usage.
It has also been discussed in wireviz#135.

The input validation is more detailed to help the user identifying
and locating invalid values. The wire color padding is now done on
the output to cover different input alternatives.
kvid added a commit to kvid/WireViz that referenced this pull request Aug 24, 2021
This was requested by designer2k2 in wireviz#219 for bgcolor usage.
It has also been discussed in wireviz#135.

The input validation is more detailed to help the user identifying
and locating invalid values. The wire color padding is now done on
the output to cover different input alternatives.
@formatc1702
Copy link
Collaborator

formatc1702 commented Aug 24, 2021

  • To make it easier choosing background colors, I suggest adding more light colors into _color_hex.

IMHO, _color_hex is useful for translating the common engineering abbreviations into hex values, not for creating a needlessly wide color palette.

Would it be possible to adapt get_color_hex() to solve #135 as well? then, users would have plenty of light colors from to choose from the HTML color names ("Azure", "Snow", "WhiteSmoke", ...) for formatting, without complicating the code and forcing made-up short names for them. Even finer color control is already possible in this PR using hex values anyway.

I see "using an obscure HTML color name as a wire color" (like tomato as in the example in #135) as an extreme edge case, and would not worry about it missing a short name.

If the request of solving #135 in this PR which has been waiting for too long, seems like too much effort, I have no problem performing a review now, and tackling HTML color names separately.

This solves the wireviz#210 suggestion to render the title row of
the graph nodes with this bgcolor.
Solves wireviz#210 completely by supporting bgcolor of both the node title
and the whole node independently using separate attributes.
Maybe not needed that much, but mainly for consistency, to support
bgcolor in all dataclasses that represent boxes in the diagram.
This was requested by designer2k2 in wireviz#219 for bgcolor usage.
It has also been discussed in wireviz#135.

The input validation is more detailed to help the user identifying
and locating invalid values. The wire color padding is now done on
the output to cover different input alternatives.
@kvid
Copy link
Collaborator Author

kvid commented Sep 25, 2021

  • To make it easier choosing background colors, I suggest adding more light colors into _color_hex.

I wrote the suggestion above before implementing hex values input, that actually removes the immediate need for more colors.

IMHO, _color_hex is useful for translating the common engineering abbreviations into hex values, not for creating a needlessly wide color palette.

I see your point, and have no problem with that. However, I also guess that some users might find it useful with a few more shades of certain colors, and have thought about how #135 could have been implemented to solve that, but it seems I don't have enough time to implement such a solution within the near future.

Would it be possible to adapt get_color_hex() to solve #135 as well? then, users would have plenty of light colors from to choose from the HTML color names ("Azure", "Snow", "WhiteSmoke", ...) for formatting, without complicating the code and forcing made-up short names for them. Even finer color control is already possible in this PR using hex values anyway.

I see "using an obscure HTML color name as a wire color" (like tomato as in the example in #135) as an extreme edge case, and would not worry about it missing a short name.

If the request of solving #135 in this PR which has been waiting for too long, seems like too much effort, I have no problem performing a review now, and tackling HTML color names separately.

I wanted to rebase this branch on top of dev, but when solving a few conflicts, I managed to end up with a circular import error in the resulting code, and therefore could not push it. I had to undo the rebase, and start over solving the same conflicts once more, but had to put it on hold for a while due to other important tasks taking all my time. Eventually, I have now finished this job, and ended up adding an extra commit to solve the circular imports.

I think it's a good idea to solve #135 in a separate PR.

@formatc1702
Copy link
Collaborator

OK, I understand. So to sum up

@formatc1702 formatc1702 merged commit 7125f28 into wireviz:dev Sep 28, 2021
formatc1702 pushed a commit that referenced this pull request Sep 28, 2021
This was requested by designer2k2 in #219 for bgcolor usage.
It has also been discussed in #135.

The input validation is more detailed to help the user identifying
and locating invalid values. The wire color padding is now done on
the output to cover different input alternatives.
@formatc1702
Copy link
Collaborator

Thanks. The commit history of this PR was so short and clean, and contained a few little useful references that I decided not to squash.

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.

3 participants