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

[feature] Define a new dataclass with color and font attributes #255

Draft
wants to merge 5 commits into
base: dev
Choose a base branch
from

Conversation

kvid
Copy link
Collaborator

@kvid kvid commented Oct 23, 2021

This solves the basic part of wireviz#225 - supporting options to specify
- Foreground/border color and text color in addition to bgcolor
- Font size is not requested in wireviz#225, but included as well
In the elements: Image, AdditionalComponent, Connector, and Cable.
Avoid the extra base part of the data structure.
@formatc1702
Copy link
Collaborator

Thanks for tackling the issue with more customizable looks! WireViz dark mode FTW!

Please bear in mind that -as you may or may not have seen-, I am working on a major refactoring on the entire WireViz codebase in #251, so I would prefer to implement this feature on top of the freshly refactored code.

The best approach to get this merged will likely look like this:

  • You can keep working on this PR based on the stable dev branch until you are satisfied and are ready to submit
  • However, please don't expect this new PR to be merged before dev has caught up with the refactoring
    • (I would like to avoid merging this in dev before, and then having to rebase the entire refactoring on top)
  • Once the refactoring is complete, we adapt the PR to work with, and rebase on top of, the refactored code
    • It would be risky to rebase already, because some upcoming changes might force you to needlessly refactor multiple times.

The refactoring should change little in the logic required for this PR and in the result for the user, but a lot in the actual implementation... but reworking it should be relatively straightforward.

@kvid
Copy link
Collaborator Author

kvid commented Oct 23, 2021

Thanks for tackling the issue with more customizable looks! WireViz dark mode FTW!

I started this branch in April, and tried a few approaches, but decided to wait until both #214 and #219 was merged, because I needed both of these to complete. I picked up the thread again 3 weeks ago, but got too busy with other things before I managed to create a PR. I observed a lot of activity these last weeks, including a release, but I had very limited time, and have not yet looked into all the new stuff.

I decided to create this as a draft PR to let you know what I have in mind. One of the reasons it's not yet completed, is that I suggest to put in inheritance in the dataclasses, but that makes certain things more complicated:

  • It might be an advantage to see how this fits together with a refactoring of all dataclasses to inherit common superclasses instead of duplicating all the common attributes, like I suggested in issue [feature] More control over wire parameters #56 (comment). I don't know yet if such a thing might also be part of your Large scale refactoring #251.
  • There are a few attributes without default value in some of the dataclasses, and that creates a problem when such a dataclass should inherit from a superclass that contains any attribute with a default value. The simplest solution is probably to assign a None default value and throw an exception if such a None value actually appear in __post_init__(), but maybe you suggest a different work-around.

Please bear in mind that -as you may or may not have seen-, I am working on a major refactoring on the entire WireViz codebase in #251, so I would prefer to implement this feature on top of the freshly refactored code.

The best approach to get this merged will likely look like this:

  • You can keep working on this PR based on the stable dev branch until you are satisfied and are ready to submit
  • However, please don't expect this new PR to be merged before dev has caught up with the refactoring

That's also what I have thought, but in case your #251 also involves an inheritance structure of dataclasses, there might exist solutions that will benefit both these PRs.

  • (I would like to avoid merging this in dev before, and then having to rebase the entire refactoring on top)

  • Once the refactoring is complete, we adapt the PR to work with, and rebase on top of, the refactored code

    • It would be risky to rebase already, because some upcoming changes might force you to needlessly refactor multiple times.

That seems reasonable. That's also one of the reasons for me to make this a draft PR at this moment.

The refactoring should change little in the logic required for this PR and in the result for the user, but a lot in the actual implementation... but reworking it should be relatively straightforward.

I look forward to have a closer look into the #251 PR when I have the time to spend.

@formatc1702
Copy link
Collaborator

I look forward to have a closer look into the #251 PR when I have the time to spend.

You're welcome to have a look anytime, but expect the unexpected (missing features, unexpected output, etc.) while the PR is in draft status ;)

@kvid kvid added enhancement New feature or request waiting Waiting for other PRs to proceed and removed enhancement New feature or request labels Jan 29, 2022
@kvid kvid mentioned this pull request May 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting Waiting for other PRs to proceed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants