Conversation
qiushihe
approved these changes
Aug 29, 2017
Collaborator
qiushihe
left a comment
There was a problem hiding this comment.
Looks good to me.
However I noticed a few small things... first there seems to be many commented out console.log statements and I think they should just be removed; second, neither package.json from the two packages have homepage set which would help with discovery once they are published so I think those should be added.
Contributor
Author
|
@qiushihe Updated - thanks! |
| "babel-preset-react": "^6.24.1", | ||
| "babel-preset-stage-2": "^6.24.1", | ||
| "eslint": "^4.5.0", | ||
| "eslint-config-metalab": "^7.0.0", |
Contributor
There was a problem hiding this comment.
You probably want 7.0.1.
be65794 to
3e8e537
Compare
izaakschroeder
approved these changes
Aug 31, 2017
Contributor
izaakschroeder
left a comment
There was a problem hiding this comment.
LGTM pending CI 🎉 Nice work!
This is a major overhaul of both the internal mechanics and public interface of the FlowTip library. The most important changes are a total separation of the layout algorithm from the React component and an a much simpler React component implementation. With the React component and layout algorithm decoupled, the stage is set for additional FlowTip implementations on other platforms. **`flowtip-core`** The new layout algorithm (now published as `flowtip-core`) is now an entirely pure function with no coupling to React or to the DOM environment. It now has very clearly defined behavior around even the most unusual layout edge cases. The algorithm is designed to serve the FlowTip React component, but exists as a separate module for both test isolation and to be available for additional uses. The algorithm also now features full test coverage using a novel ascii renderer combined with Jest snapshots. **`flowtip-react-dom`** The previous `flowtip` component module is succeeded by `flowtip-react-dom`. It provides a friendly React interface to the underlying `flowtip-core` algorithm with specific bindings to DOM events and the React component lifecycle. The main architectural motivations for the design of the new of the React component are simplicity and declarative composability with a simple interface inspired by modules like `react-motion`, `react-router`, and `redux-form`. The component behaves as an inline "decorator" which enhances a provided `Content` component with tooltip layout behavior. Because it avoids owning any unnecessary state, it requires a parent component (or global state e.g. Redux) to manage the target rect and "active" state. It also forgoes implementing what could be seen as valuable core features for the sake of simplicity. Additional libraries like `react-resize-observer`, `react-motion`, `tj/react-click-outside`, and `focus-trap-react` are recommended to compose a full user experience. Since `flowtip-react-dom` renders no markup of it's own and defers all rendering responsibility to consumer defined components, composition with the mentioned libraries should be without conflict. This minimal approach offers the best balance of core features and compatibility with various consumer use cases.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is a major overhaul of both the internal mechanics and public interface of the FlowTip library.
The most important changes are a total separation of the layout algorithm from the React component and an a much simpler React component implementation.
With the React component and layout algorithm decoupled, the stage is set for additional FlowTip implementations on other platforms.
flowtip-coreThe new layout algorithm (now published as
flowtip-core) is now an entirely pure function with no coupling to React or to the DOM environment. It now has very clearly defined behavior around even the most unusual layout edge cases.The algorithm is designed to serve the FlowTip React component, but exists as a separate module for both test isolation and to be available for additional uses.
The algorithm also now features full test coverage using a novel ascii renderer combined with Jest snapshots.
flowtip-react-domThe previous
flowtipcomponent module is succeeded byflowtip-react-dom. It provides a friendly React interface to the underlyingflowtip-corealgorithm with specific bindings to DOM events and the React component lifecycle.The main architectural motivations for the design of the new of the React component are simplicity and declarative composability with a simple interface inspired by modules like
react-motion,react-router, andredux-form.The component behaves as an inline "decorator" which enhances a provided
Contentcomponent with tooltip layout behavior. Because it avoids owning any unnecessary state, it requires a parent component (or global state e.g. Redux) to manage the target rect and "active" state.It also forgoes implementing what could be seen as valuable core features for the sake of simplicity. Additional libraries like
react-resize-observer,react-motion,tj/react-click-outside, andfocus-trap-reactare recommended to compose a full user experience. Sinceflowtip-react-domrenders no markup of it's own and defers all rendering responsibility to consumer defined components, composition with the mentioned libraries should be without conflict.This minimal approach offers the best balance of core features and compatibility with various consumer use cases.
TODO:
/cc @qiushihe @izaakschroeder