feat(popperjs): use popperjs instead of tether#561
Conversation
|
Popper.js exports the placements as Popper.placements No need to hard code them in this code |
virgofx
left a comment
virgofx
left a comment
There was a problem hiding this comment.
Code-wise looks excellent. Clean implementation of Popper and a lot of test refactoring to Jest. Wondering if we made any speed improvements in the test framework?
Looks good. Need to test it live
| @@ -100,7 +100,7 @@ npm install --save reactstrap react-addons-transition-group react-addons-css-tra | |||
| <p>Check out the demo <a href="http://output.jsbin.com/dimive/latest">here</a></p> | |||
There was a problem hiding this comment.
Looks like this link no longer works. Need to figure out replacement.
I know this is carry over from existing docs not related to popper but I remember see it .. Might as well either remove it.
There was a problem hiding this comment.
We'll have to update this later, after v5 so that the example can include v5 things. I am thinking that we should use webpackbin or codesandbox.
| outline: PropTypes.bool, | ||
| tag: PropTypes.oneOfType([PropTypes.func, PropTypes.string]), | ||
| getRef: PropTypes.oneOfType([PropTypes.func, PropTypes.string]), | ||
| innerRef: PropTypes.oneOfType([PropTypes.func, PropTypes.string]), |
There was a problem hiding this comment.
Curious ... I know getRef() was used in a lot of places. Are we switching to innerRef() ? It actually makes more sense intuitively as I ran into this issue during development and forgot the actual ref was getRef().
There was a problem hiding this comment.
This was to line up with other libraries. I considered keeping both, aliasing one to the other but thought doing so might cause some complications.
There was a problem hiding this comment.
Cool, let's just use innerRef and make sure all other components use that.
| return <Manager {...attrs} className={classes}>{}</Manager>; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Codewise, even cleaner and easier to read. Removed unnecessary logic in a few areas. 👍
| clearTimeout(this._hideTimeout); | ||
| this._hideTimeout = undefined; | ||
| } | ||
|
|
There was a problem hiding this comment.
Will all the show/hide timeout logic ... will this also fix the few issues we have with fast enable/disabling of popovers/tooltips where it was previously crashing? If not, maybe test with delays of 2000 (2s) to make sure we can close those issues too.
There was a problem hiding this comment.
Animations still need to be reworked, but the way this works it should better handle timeouts and toggles. I don't really want to have a long running test; there are tests to ensure the timeouts are cleared.
| import classNames from 'classnames'; | ||
| import TetherContent from './TetherContent'; | ||
| import { getTetherAttachments, mapToCssModules, omit, tetherAttachements } from './utils'; | ||
| import Popper from './PopperContent'; |
There was a problem hiding this comment.
I know we switched from Tether to Popper. Keeping it as PopperContent makes sense to avoid confusion with main popper.js library; however, since this is not really an exposed component persay, it's cleaner to just use it as Popper as you did here. If so ... should we rename ./PopperContent to Popper. Otherwise, maybe keep the class name as PopperContent for consistency.
|
many thanks! i am very much anticipating this release! Is there a planned release date for v5? |
|
@Sathras I would like to get #557 in before v5, though I would like to get a beta release asap, but I need @eddywashere to help with that. |
Add tests Update existing tests (jest has been upgraded) improve code coverage Breaking change: getRef has been renamed to innerRef to line up with other libraries. Breaking change: tether props have been removed.
4 participants
This replaces tether and TetherConent with PopperJS via react-popper.
Things touched:
Dropdown(andDropdown<Things>)TooltipPopoverIt also includes an update to
react-scriptsas I needed new features. This updates other things, such asjestas well aseslint. This causes some of the tests to change due to the jest 19/20 changes and caused other code files to change due to new eslint rules.Test have been updated and coverage has been improved.
Docs have been updated
I have done my best to keep the expose API (props and such) the same, but some things such as
tetherandtetherRefprops are no longer relevant and have been removed. Other props have been changed to better align with other components.