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

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

Conversation

henri-edinb
Copy link

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?

@henri-edinb henri-edinb changed the title Update Graph.jsx Allow background-color and border-style to be set on Graph config Jan 16, 2021
@@ -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

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.

None yet

3 participants