Skip to content
This repository has been archived by the owner on Feb 19, 2022. It is now read-only.

Initial API #1

Merged
merged 34 commits into from Mar 22, 2016
Merged

Initial API #1

merged 34 commits into from Mar 22, 2016

Conversation

tptee
Copy link
Contributor

@tptee tptee commented Mar 15, 2016

The initial implementation of radium-constraints should be a good enough kickstart to begin integration into victory. This library came to life as a solution for multiple layout problems in victory:

FormidableLabs/victory-core/issues/17
FormidableLabs/victory-chart/issues/122
FormidableLabs/victory-chart/issues/124
FormidableLabs/victory-chart/issues/121
FormidableLabs/victory-chart/issues/33

You can track integration with Victory here:
FormidableLabs/victory/issues/225

From the README:

Radium Constraints introduces the power of constraint-based layout to React. Declare simple relationships between your visual components and let the constraint solver generate the optimum layout.

Radium Constraints handles DOM and SVG elements and is the perfect alternative to manual layout when building SVG data visualizations. Radium Constraints is the bedrock for exciting new enhancements to Victory.

Learn more about constraint-based layouts:
http://overconstrained.io/
https://developer.apple.com/library/ios/documentation/UserExperience/Conceptual/AutolayoutPG/
https://constraints.cs.washington.edu/solvers/cassowary-tochi.pdf
https://gridstylesheets.org/

@tptee
Copy link
Contributor Author

tptee commented Mar 17, 2016

Running into nondeterministic test failures with the PhantomJS + isparta-loader combo. Will probably need to upgrade PhantomJS (╯°□°)╯︵ ┻━┻

@tptee
Copy link
Contributor Author

tptee commented Mar 21, 2016

There's a fair bit left to do (documented in the Roadmap section of the updated README) but I think this is ready for an initial review/pre-alpha. @coopy @alexlande @boygirl @baer @divmain @exogen et al

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 6e29cbe on feature-initial-api into * on master*.

- Create build tool to pre-calculate initial layouts.
- Decide on a distribution strategy for the WebWorker code (preference on inlining).
- Decide on an animation strategy (requires support for removing constraints).
- Allow for self-referential subviews in the constraint props array without using the subview string.
Copy link

Choose a reason for hiding this comment

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

This README is 💯 - usage section, especially. One question, however: will the DSL be documented?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes indeed! I'm debating on docstrings vs. just writing it out, thoughts?

Copy link

Choose a reason for hiding this comment

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

If it were just for me, I'd vote for docstrings - I like reading through the source. But since this is a public package, I guess explicit documentation in the README would be better.

@boygirl
Copy link

boygirl commented Mar 21, 2016

@tptee this is looking great! Can you add a todo for an initial render strategy for when layouts aren't pre-calculated?

@tptee
Copy link
Contributor Author

tptee commented Mar 21, 2016

@boygirl Sure thing!

};

export default Object.keys(transformers).reduce((acc, key) => {
return {...acc, [key]: Subview(key, transformers[key])};
Copy link
Member

Choose a reason for hiding this comment

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

GLOBAL: Mutate instead of extending where possible.

This:

export default Object.keys(transformers).reduce((acc, key) => {
  return {...acc, [key]: Subview(key, transformers[key])};
}, {});

translates to:

"use strict";

Object.defineProperty(exports, "__esModule", {
  value: true
});

var _extends = Object.assign || function (target) { for (var i = 1; i < arguments.length; i++) { var source = arguments[i]; for (var key in source) { if (Object.prototype.hasOwnProperty.call(source, key)) { target[key] = source[key]; } } } return target; };

function _defineProperty(obj, key, value) { if (key in obj) { Object.defineProperty(obj, key, { value: value, enumerable: true, configurable: true, writable: true }); } else { obj[key] = value; } return obj; }

exports.default = Object.keys(transformers).reduce(function (acc, key) {
  return _extends({}, acc, _defineProperty({}, key, Subview(key, transformers[key])));
}, {});

which is a lot of object extending / re-recreation.

Instead, doing a shallow straight mutation like:

export default Object.keys(transformers).reduce((acc, key) => {
  acc[key] = Subview(key, transformers[key]);
  return acc;
}, {});

translates to:

"use strict";

Object.defineProperty(exports, "__esModule", {
  value: true
});
exports.default = Object.keys(transformers).reduce(function (acc, key) {
  acc[key] = Subview(key, transformers[key]);
  return acc;
}, {});

and notably avoids all the object re-creation / extending...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 . Less "tricky" and more readable as well

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 4caab16 on feature-initial-api into * on master*.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 4caab16 on feature-initial-api into * on master*.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 0778a55 on feature-initial-api into * on master*.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 0778a55 on feature-initial-api into * on master*.

@tptee
Copy link
Contributor Author

tptee commented Mar 22, 2016

Thanks for your help, everyone! I've integrated your suggestions and ticketed out future work/enhancements. I'm going to merge this so that we can start reviewing smaller PRs 😜

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 445aedf on feature-initial-api into * on master*.

tptee added a commit that referenced this pull request Mar 22, 2016
@tptee tptee merged commit 8ec35ca into master Mar 22, 2016
@tptee tptee deleted the feature-initial-api branch March 22, 2016 06:11
@tptee tptee mentioned this pull request Mar 22, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants