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

Update hierarchy comps #146

Merged
merged 4 commits into from
Sep 12, 2017
Merged

Conversation

trainorpj
Copy link
Contributor

This is related to #139

This PR does the following (each statement applies to both <Tree /> and <Cluster /> components:

  1. Adds className to each <Group /> component.
  • Is this where the className prop should go? I didn't see any other reasonable place to put it.
  1. Adds <DefaultLink /> and <DefaultNode /> components to each component, along with basic tests for each.
  • These render just fine--let me know if there are issues with them.
  • I don't know much about testing components, so let me know if we need more comprehensive tests right now 🃏

This PR does not

  1. Add defaults to the size, nodeSize, and separation props.
  • The defaults are set in the corresponding d3 functions. The questions is, are we setting defaults at the highest or lowest level they're called at?
    • For example, there is no default for the top prop in either component--the prop gets passed to <Group /> which does have a default for top. So the default is set there (that's what I mean by "lowest level". I think this is the best way to do it.
  1. Update the Readme.
  • I can do this once the above points are addressed (let me know and I can add them before you merge 👍)

This is my first contribution to the codebase outside of documentation, so let me know if there's something I missed. For example, it doesn't have today's commits to master (I probably need to do that, right? 😬).

@hshoff hshoff added this to the v0.0.139 milestone Sep 10, 2017
</Group>
);
})}
<Group top={top} left={left} className={className}>
Copy link
Member

Choose a reason for hiding this comment

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

let's change this to

className={cx('vx-cluster', className)}

that way it has a default class and folks can add their own

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got them both. I also found a (small) bug in my code, adding className where it has no business being 🐛

</Group>
);
})}
<Group top={top} left={left} className={className}>
Copy link
Member

Choose a reason for hiding this comment

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

same here

className={cx('vx-tree', className)}

@trainorpj
Copy link
Contributor Author

I'll update the readme when this is merged (I need to have working links to the components).

@hshoff hshoff merged commit abc6378 into airbnb:master Sep 12, 2017
@trainorpj trainorpj deleted the update-hierarchy-comps branch September 12, 2017 22:56
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.

2 participants