-
Notifications
You must be signed in to change notification settings - Fork 231
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
Allow background-color and border-style to be set on Graph config #429
base: master
Are you sure you want to change the base?
Conversation
@@ -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, |
There was a problem hiding this comment.
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,
};
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 thesvgStyle
. - Adjustments in the
Sandbox.js
might be required, I might be able to help with that later on
Very specific solution for issue #428. Not really sure if it will work but seems reasonable. Maybe a more general solution would be to have the whole style object in config?