Prepare moving to context API #259
Conversation
|
|
||
| }; | ||
|
|
||
| export default I13nComponent; |
There was a problem hiding this comment.
This component will be the main class handling different functions (replacing ComponentSpecs)
src/utils/setupI13n.js
Outdated
| // getI13nNode: this.getI13nNode.bind(this), | ||
| // reactI13nInstance: this._reactI13nInstance | ||
| // }; | ||
| // } |
There was a problem hiding 8000 this comment.
We probably don't need to pass props anymore for accessing these function
| let href = ''; | ||
|
|
||
| // return and do nothing if the handler is append on a component without I13nMixin | ||
| if (!self.executeI13nEvent) { |
There was a problem hiding this comment.
make no sense for a handler to assume caller method
| * @method getInstance | ||
| * @return the ReactI13n instance | ||
| */ | ||
| static getInstance() { |
There was a problem hiding this comment.
this is for people accessing outside of React tree, moving to another export method and change not to pollute window
src/libs/ComponentSpecs.js
Outdated
| @@ -47,7 +26,7 @@ const staticSpecs = { | |||
| // remove propTypes for production build | |||
| propTypes: { | |||
| component: PropTypes.oneOfType([PropTypes.string, PropTypes.func]), | |||
| model: PropTypes.oneOfType([PropTypes.object, PropTypes.func]), | |||
| // model: PropTypes.oneOfType([PropTypes.object, PropTypes.func]), | |||
There was a problem hiding this comment.
legacy props, planned to remove it
src/libs/ComponentSpecs.js
Outdated
| } | ||
| // if (self.props.scanLinks && self.props.scanLinks.enable) { | ||
| // self._scanLinks(); | ||
| // } |
There was a problem hiding this comment.
slowly migrate logic to hooks and the new component entry point, comment out logic I already migrated
src/libs/ComponentSpecs.js
Outdated
| // }); | ||
| // self._getReactI13n().execute('created', { i13nNode }); | ||
| // }); | ||
| // }, | ||
|
|
||
| /** | ||
| * _shouldFollowLink, provide a hook to check followLink. |
There was a problem hiding this comment.
We never expose shouldFollowLink to any documentation, probably only use internally. This should either be a props or something else if we want to support it. (functional component won't work anyway)
| const self = this; | ||
| if (self._subI13nComponents && self._subI13nComponents.length > 0) { | ||
| self._subI13nComponents.forEach((subI13nComponent) => { | ||
| subI13nComponent.componentClickListener.remove(); |
There was a problem hiding this comment.
fun fact, this should be unsubscribe instead of remove
https://github.com/yahoo/subscribe-ui-event
tests/functional/i13n.spec.js
Outdated
| const result = window._reactI13nInstance.getRootI13nNode().getMergedModel(); | ||
| expect(result).to.eql({ sec: 'default-section-name', page: 'test-page' }); | ||
| }); | ||
|
|
||
| it('should fire a pageview', () => { | ||
| it.only('should fire a pageview', () => { |
There was a problem hiding this comment.
will enable test one by one to make sure all cases are covered (except if we have breaking changes)
| }); | ||
|
|
||
| it('should not follow it if follow is set to false', (done) => { |
There was a problem hiding this comment.
duplicated test
| mockComponent = { | ||
| props: { | ||
| href: 'foo' | ||
| }, |
There was a problem hiding this comment.
this is actually a bug which hides BUTTON logic.
| // Do not trigger navigate action. Let browser handle it natively. | ||
| if (!href || (href && href[0] === '#')) { |
There was a problem hiding this comment.
this is actually an existing bug, when a button-ish type tag is used, we will never have href, and we always set the value to false even there is a form. the callback below will never be executed.
Implementation
augmentComponentandcomponentSpec, several issues for the current implementationrootandchild/leavecomponents but just cherrypick some of the prototype/static method for root, which doesn't make much sense to share the logic as most of the logic just for child node, and it increase the complexity greatlythis.shouldFollowLink, but in our code logic we are assuming the method exists (black magic).Context.Providerto overrides theI13nNodeandParentNodebut share those common functioncomponentSpecto hooks so the lifecycle will be much cleaner, and we can separate hooks like plugin (useScanLinks, useViewportDetect` ...) and each hooks handle its own logic, the main I13nComponent file will be much cleaner to readBreaking changes
intersection observer, you must require polyfillcreateI13nNode, we will passinnerPropsto the wrapped component, and you must attach to your render functionReactI13n.getInstance, you can directlyimport { getInstance } from 'react-i13nthis.context.i13n, we will use directly react contextimport { I13nContext } from 'react-i13nwhich exposeexecuteEventI confirm that this contribution is made under the terms of the license found in the root directory of this repository's source tree and that I have the authority necessary to make this contribution on behalf of its copyright owner.