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

add I13nUtils mixin / add setParentNode in I13nNode #22

Merged
merged 1 commit into from
Jun 15, 2015
Merged

Conversation

kaesonho
Copy link
Contributor

@redonkulus @lingyan

provide i13n utils mixins for users to have a easier way to access i13n node and execute i13n event
(move some shared functions to i13nUtils, to provide a small mixin for i13n functionalities)

getI13nNode

when we use createI13nNode to decorate a node, for example createI13nNode(Foo), it generate a template like

<I13nFoo> // everything about i13n and creating i13nNode happens here
     <Foo /> // original Foo, currently Foo cannot access anything about I13nFoo
</13nFoo>

in Foo, we might have some case want to access the i13n node, so provide getI13nNode to get the nearest i13nNode in context passed from parents.

In the end, they are do something like

var I13nUtils = require('react-i13n').I13nUtils;
var DemoComponent = React.createClass({
    mixins: [I13nUtils],
    displayName: 'DemoComponent',
    render: function() {
        // this.getI13nNode() to access the i13nNode created by createI13nNode
    }
});
var I13nDemoComponent = createI13nNode(DemoComponent);

executeI13nEvent

previously users will have to fire events like

var DemoComponent = React.createClass({
    displayName: 'DemoComponent',
    componentDidMount: function () {
         ReactI13n.getInstance.execute('pageview', {});
    }
});

now we provide executeI13nEvent for them, we can add some error handling and get the i13nNode into payload for them, (I will update all document that users should use this directly)

var I13nUtils = require('react-i13n').I13nUtils;
var DemoComponent = React.createClass({
    mixins: [I13nUtils],
    displayName: 'DemoComponent',
    componentDidMount: function () {
        // executeI13nEvent will find the i13nNode and append to the payload for you, which means the final payload will be
        // {i13nNode: [theI13nNode], foo: 'bar'}
        this.executeI13nEvent('someEventName', {foo: 'bar'}, function callback() {
            // callback
        });
    }
});

add setParentNode

Users might have case that they want to dynamically change the I13nTree structor, they can to that by changing parent node of some I13nNode

this is actually for mail's use case, they have some component which is not a parent in the react dom tree, but they want to inherit the data of it, so that they can do this by changing the i13n tree structor.

Warning fix

break immediately when we don't have any handlers for the event, instead of showing something like ReactI13n handler timeout in 1000ms. +951ms

@yahoocla
Copy link

CLA is valid!

@kaesonho kaesonho changed the title add I13nUtils mixin add I13nUtils mixin / add setParentNode in I13nNode Jun 10, 2015
@kaesonho kaesonho force-pushed the addI13nUtils branch 2 times, most recently from 9d17f7c to 429cc54 Compare June 10, 2015 05:57
@@ -0,0 +1,40 @@
## I13nUtils

The `I13Utils` mixin provides util functions for you to access the i13n nodes and execute i13n functionalities
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not just call it I13nMixin?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, exactly, if you are using I13nMixin, we can access the i13nNode by accessing this._i13nNode, but I think it's even better to have a function like this.getI13nNode()

I13Utils is a subset of I13nMixin, so using I13nMixin can still get that function

Copy link
Contributor

Choose a reason for hiding this comment

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

I think its confusing to have I13Utils and I13nMixin, its not clear when to use which one. Maybe just merge them and always use I13nMixin?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if we combine that all the functionalities in a mixin, that mixin will always created a i13nNode for that component, and adding everything seems heavy if we just want some functionalities like executeI13nEvent or getI13nNode,

actually I didn't see any reason to expose I13nMixin to users, how about remove it from the document and get users to use createI13nNode anyway?

Copy link
Contributor

Choose a reason for hiding this comment

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

Thats fine if its not needed. It was mainly for components that didn't want to use createI13nNode right?

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, it's exposed for the components don't want to use createI13nNode, so far as I know, people always use createI13nNode,

for me, createI13nNode actually makes more sense for it's naming, (I13nMixin doesn't provide enough information about what it does), and it let users to set options easily https://github.com/yahoo/react-i13n/blob/master/docs/api/createI13nNode.md#createi13nnodecomponent-options,

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, thats ok. So whats the end result here? Will I13Utils just become the mixin now?

@kaesonho
Copy link
Contributor Author

so as our discussion,

  1. remove I13nUtils
  2. create an object i13n, including i13nNode and execute, then pass is via both props and context
  3. hide I13nMixin from document.

@kaesonho
Copy link
Contributor Author

updated according to previous comment,
still keep I13nUtils as a internal library since both createI13nNode and setupI13n need that.

now using createI13nNode and setupI13n, users will get this.props.i13n for free, and users can get it via this.context.i13n as well.

displayName: 'DemoComponent',
render: function() {
// this.props.i13n.getI13nNode() to access the i13nNode created by createI13nNode
// this.props.executeEvent() to execute i13n event
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldnt it be this.props.i13n.executeEvent

@redonkulus
Copy link
Contributor

squash and 👍

Lets make sure all examples use the new this.props.i13n syntax

kaesonho added a commit that referenced this pull request Jun 15, 2015
add I13nUtils mixin / add setParentNode in I13nNode
@kaesonho kaesonho merged commit 669afa3 into master Jun 15, 2015
@kaesonho kaesonho deleted the addI13nUtils branch June 15, 2015 23:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants