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

feat($core): adds support for tension, beta, alpha in curves #81

Merged

Conversation

t-mullen
Copy link
Contributor

@t-mullen t-mullen commented Nov 8, 2018

Adds support for D3 curve modification methods like tension, alpha and
beta. Re-done because of the broken Git forks.

Closes #39

Updates

  • Adds in support for all D3 curve modifying methods (.tension(), .alpha(), .beta(), any any other similar methods).

Demo screenshot or recording

The demo curveNatural has been replaced with the curveCardinal with a modified tension (it is visually identical.)

Review checklist (for reviewers only)

  • Demos all features
  • Documented/annotated
  • Matches UI/UX specs
  • Meets the code style guide
  • Accessible
  • Mobile first (responsive)
  • RTL support (bidirectional text)
  • Performant (limited bloat)

@t-mullen
Copy link
Contributor Author

t-mullen commented Nov 8, 2018

The idea to support the dual curve option instead of of two options gets pretty weird with the typings. I'm not sure if it's the best call.

@netlify
Copy link

netlify bot commented Nov 8, 2018

Deploy preview for kind-hypatia-c6b5c4 ready!

Built with commit 09437dc

https://deploy-preview-81--kind-hypatia-c6b5c4.netlify.com

@theiliad
Copy link
Member

theiliad commented Nov 9, 2018

I think we need to have a discussion here.

There are 2 ways to do this:
1.
image

image
image

@t-mullen @zvonimirfras @cal-smith which approach looks more ideal to you?

Aside from that the implementation works great, although I would suggest we throw a console.error when user enters curve options that don't exist.

@cal-smith
Copy link
Contributor

I think 2 overall ... it's a little more complex, but it does a better job of keeping everything localized.

That said, does the rest of the library use x + xOptions? Or would the plan be to refactor any of those instances to the second style as well?

@theiliad theiliad changed the title feat($core): adds support for tension, beta, alpha in curves [WIP] - feat($core): adds support for tension, beta, alpha in curves Nov 12, 2018
@theiliad
Copy link
Member

@cal-smith I agree about approach number 2. Nope, usually we don't do x + xOptions.

@theiliad
Copy link
Member

Any updates here @t-mullen?

@t-mullen
Copy link
Contributor Author

The decided-upon API has been implemented.

Copy link
Member

@theiliad theiliad left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@theiliad
Copy link
Member

theiliad commented Dec 29, 2018

@zvonimirfras @cal-smith ready to merge

@theiliad theiliad changed the title [WIP] - feat($core): adds support for tension, beta, alpha in curves feat($core): adds support for tension, beta, alpha in curves Jan 3, 2019
@theiliad theiliad self-assigned this Jan 3, 2019
@zvonimirfras zvonimirfras merged commit 8de48bd into carbon-design-system:master Jan 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants