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

Allow background-color and border-style to be set on Graph config #429

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Allow background-color and border-style to be set on Graph config #429

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c6a5ba6
Update Graph.jsx
henri-edinb Jan 15, 2021
381425d
add a callback for 'while dragging' node
henri-edinb Nov 20, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/components/graph/Graph.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -664,6 +664,8 @@ export default class Graph extends React.Component {
const svgStyle = {
height: this.state.config.height,
width: this.state.config.width,
'background-color': this.state.config.backgroundColor,
Copy link
Owner

Choose a reason for hiding this comment

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

We could potentially expose the full object style and simply spread it on the svgStyle. What do you think about this? @LonelyPrincess @antoninklopp @terahn?

My suggestion would be something like a style object at the root config level this.state.config.style, that we could simply feed to the svgStyle.

// I would just keep width/height segregated into their own props for retro-compatibility purposes
const svgStyle = {
  height: this.state.config.height,
  width: this.state.config.width,
  ...this.state.config.style,
};

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think that this is definitely something that the configuration could provide

Copy link
Owner

Choose a reason for hiding this comment

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

What do you think, @henri-edinb? Are you up to do the change? This would be a much more meaningful contribution. Clients will be able to pass in any style to apply to the background.

Copy link
Author

Choose a reason for hiding this comment

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

to be honest Im quite new to react so Im not sure exactly what needs to be done. setting

const svgStyle = { height: this.state.config.height, width: this.state.config.width, ...this.state.config.style, };

would do it? or is there other adjustments needed?

Copy link
Owner

Choose a reason for hiding this comment

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

I'm not able to give the proper guidance at the moment @henri-edinb. If someone can chime in please feel free to help @antoninklopp. What we're looking at here is:

  • Add a new config object config.style which is an optional Object. By default, it should be empty in the default graph config.
  • After that, we can perform the change that Henri is mentioning, spreading the style object on the svgStyle.
  • Adjustments in the Sandbox.js might be required, I might be able to help with that later on

'border-style': this.state.config.border,
};

const containerProps = this._generateFocusAnimationProps();
Expand Down