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 fontColor as a configuration option for node's <text> fill property #64

Merged
merged 8 commits into from
Apr 11, 2018

Conversation

dmmulroy
Copy link
Contributor

This enables users to set a fontColor under the node configuration option that will set the Node Components svg elements fill color.

dy: CONST.NODE_LABEL_DY,
fontSize: this.props.fontSize,
fontWeight: this.props.fontWeight,
fill: this.props.fontColor,
Copy link
Contributor Author

@dmmulroy dmmulroy Apr 10, 2018

Choose a reason for hiding this comment

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

Prettier ran and formatted this file so it's hard to tell, but this is the main change in this file (the addition of fill on textProps)

Copy link
Owner

Choose a reason for hiding this comment

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

Regarding the indentation can you please change the file react-d3-graph/.prettierrc.js? It has a typo 😄 , tabWidt should be tabWidth: 4, this should make prettier reindent the file correctly. Thanks!

@dmmulroy
Copy link
Contributor Author

dmmulroy commented Apr 10, 2018

I'll get to this tonight and fix the tests

@danielcaldas
Copy link
Owner

Nice one @dmmulroy 👌, I'll complete the code review asap.

@dmmulroy
Copy link
Contributor Author

Fixed up the tests, everything should be good to go!

@@ -59,6 +59,7 @@
* property for all nodes' labels.
* @param {string} [node.fontWeight='normal'] - {@link https://developer.mozilla.org/en/docs/Web/CSS/font-weight?v=control|font-weight}
* property for all nodes' labels.
* @param {string} [node.fontWeight='normal'] - fill color for node's <text> svg label
Copy link
Owner

Choose a reason for hiding this comment

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

Just some minor details here:

  1. node.fontColor='black' instead of node.fontWeight;
  2. Properties are ordered alphabetically, so fontColor should be on top of fontWeight (both here and in the config object)

@@ -3,7 +3,7 @@
object-assign
Copy link
Owner

Choose a reason for hiding this comment

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

Please check out this change, I'll generate the sandbox bundle upon a new rd3g release, thanks

@@ -294,6 +294,7 @@ function buildNodeProps(node, config, nodeCallbacks = {}, highlightedNode, highl
const dx = fontSize * t + nodeSize / 100 + 1.5;
const strokeWidth = highlight ? config.node.highlightStrokeWidth : config.node.strokeWidth;
const svg = node.svg || config.node.svg;
const fontColor = config.node.fontColor || 'green';
Copy link
Owner

Choose a reason for hiding this comment

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

Please use the following:

const fontColor = node.fontColor || config.node.fontColor;

This way each node will be able to override the global defined color (the global is the on in config.node.fontColor)

@@ -27,6 +27,7 @@ import nodeHelper from './node.helper';
* fontSize=10
* dx=90
* fontWeight='normal'
* fontColor='black'
Copy link
Owner

Choose a reason for hiding this comment

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

Place fontColor on top of fontWeight

@@ -22,6 +22,7 @@ exports[`Snapshot - Node Component should match snapshot 1`] = `
<text
dx=".90em"
dy=".35em"
fill={undefined}
Copy link
Owner

Choose a reason for hiding this comment

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

Please check whether this value should have value black

@dmmulroy
Copy link
Contributor Author

Changes should be in

@danielcaldas danielcaldas merged commit e4ef748 into danielcaldas:master Apr 11, 2018
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

2 participants