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 extended Theme support for Edge and Node #145

Closed
wants to merge 1 commit into from

Conversation

AnWeber
Copy link

@AnWeber AnWeber commented Nov 8, 2023

PR Checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)
    • did not find any tests. updated Storybook
  • Docs have been added / updated (for bug fixes / features)

PR Type

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • Other... Please describe:

What is the current behavior?

only Node specific fill Color is supported

Issue Number: N/A

What is the new behavior?

Different colors for fill, activeFill,... can be defined for each node and edge

add extended support for Node and Edge specific Theme support

  • added theme Property to GraphNode
  • added theme Property to GraphEdge
  • deprectated fill Property in GraphNode (Hint to node.theme.fill added)
  • updated Storybook to showcase extended Color Support

Does this PR introduce a breaking change?

[x] Yes
[ ] No

I would deprecated previous fill Property. If this is not intended, deprecation hints can be removed

Copy link
Member

@amcdnl amcdnl left a comment

Choose a reason for hiding this comment

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

Few minor ?s / tweaks

@@ -51,10 +53,32 @@ export interface GraphNode extends GraphElementBaseAttributes {
* Fill color for the node.
*/
fill?: string;
theme?: {
Copy link
Member

Choose a reason for hiding this comment

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

You should be able to access the theme via the hook.

@@ -60,6 +82,23 @@ export interface GraphEdge extends GraphElementBaseAttributes {
* Target ID of the node.
*/
target: string;

theme?: {
Copy link
Member

Choose a reason for hiding this comment

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

You should be able to access the theme via the hook.

*/
fill?: string;
theme?: {
Copy link
Member

Choose a reason for hiding this comment

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

You should be able to access the theme via the hook.

@@ -46,8 +46,30 @@ export interface GraphNode extends GraphElementBaseAttributes {

/**
* Fill color for the node.
* @deprecated use theme.fill
*/
fill?: string;
Copy link
Member

Choose a reason for hiding this comment

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

Kinda have to keep this for node specific colors though we could extend it to support a callback.

@amcdnl amcdnl added the enhancement New feature or request label Nov 13, 2023
@AnWeber AnWeber requested a review from amcdnl November 13, 2023 19:16
@amcdnl
Copy link
Member

amcdnl commented Dec 16, 2023

@AnWeber - Happy to merge if you can resolve some of those items.

@AnWeber
Copy link
Author

AnWeber commented Dec 18, 2023

@AnWeber - Happy to merge if you can resolve some of those items.

@amcdnl I can gladly incorporate the comments, but I don't directly understand what I should change and how. What do you mean by mentioning You should be able to access the theme via the hook.
The change is explicitly that I can use a different theme/style per node. Access to a central theme via hook is not helpful here.

@amcdnl
Copy link
Member

amcdnl commented Feb 19, 2024

Going to close this for now given the conflicts. I'm still happy to accept this we just need to make sure the pattern follows.

@amcdnl amcdnl closed this Feb 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants