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

Isvalidelement #24

Merged
merged 3 commits into from Feb 14, 2017
Merged

Isvalidelement #24

merged 3 commits into from Feb 14, 2017

Conversation

KlonD90
Copy link
Contributor

@KlonD90 KlonD90 commented Feb 13, 2017

#21 implement isValidElement

@coveralls
Copy link

coveralls commented Feb 13, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling eb646d3 on KlonD90:isvalidelement into fadf8dc on alt-j:master.

src/index.js Outdated
},

/**
* @param {RenderElement} element
Copy link
Owner

Choose a reason for hiding this comment

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

{*} element

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm?

Copy link
Owner

Choose a reason for hiding this comment

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

This method can take any type of argument.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Get it.

src/index.js Outdated
*/
isValidElement: function (element) {
return element === null ||
(typeof element === 'object' && typeof element.type !== 'undefined' && typeof element.props === 'object');
Copy link
Owner

Choose a reason for hiding this comment

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

  1. we can add small function isObject, to make code more readable
  2. element.hasOwnProperty('type')

what do you think about that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. isObject equal typeof object == 'object'?
  2. That's good.

Copy link
Owner

Choose a reason for hiding this comment

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

  1. yep

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i write it now

    return object !== null && typeof object === 'object' && !Array.isArray(object);

@sigorilla sigorilla self-requested a review February 14, 2017 16:26
src/index.js Outdated

/**
* @param {RenderElement} element
* @returns {boolean} isValid
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it should be Boolean (in Title case)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok. I think about it like a primitive, but it's a Boolean so that's right.

…replace type check with isObject in isValiElement, replace not undefined check with hasOwnProperty type in isValidElement
@KlonD90
Copy link
Contributor Author

KlonD90 commented Feb 14, 2017

Fix change requests. Please review ._.

@coveralls
Copy link

coveralls commented Feb 14, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling 77a2f46 on KlonD90:isvalidelement into fadf8dc on alt-j:master.

@alt-j alt-j merged commit 97fa9b0 into alt-j:master Feb 14, 2017
@alt-j
Copy link
Owner

alt-j commented Feb 14, 2017

thanks for contribution 🎉

@KlonD90
Copy link
Contributor Author

KlonD90 commented Feb 14, 2017

Thanks for release 🙆

@KlonD90 KlonD90 deleted the isvalidelement branch February 14, 2017 19:57
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.

None yet

4 participants