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.

Sign up for GitHub

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

Jump to bottom

feat: implement ReCaptcha component #317

Merged

LeandroTorresSicilia merged 3 commits into master from implement-recaptcha

Oct 21, 2018

Collaborator

AlejandroYanes commented Oct 20, 2018

fixes #290


          feat: implement ReCaptcha component

ce7f76b

LeandroTorresSicilia reviewed

View reviewed changes

src/components/ReCaptcha/index.js Outdated

+                  const getUrl = () => {
+                      if (lang) {
+                          return `https://www.google.com/recaptcha/api.js?render=explicit&hl=${lang}`;
+                      }

Member

LeandroTorresSicilia Oct 21, 2018

put this in other file(get-url.js) in order to test it and then below call it as getUrl(lang)

LeandroTorresSicilia reviewed

View reviewed changes

src/components/ReCaptcha/index.js

+                  /** The color theme of the widget. */
+                  theme: PropTypes.oneOf(['light', 'dark']),
+                  /** The size of the widget. */
+                  size: PropTypes.oneOf(['normal', 'compact']),

Member

LeandroTorresSicilia Oct 21, 2018

here you forgot to pass a tabIndex prop that defaults to 0

LeandroTorresSicilia reviewed

View reviewed changes

src/components/ReCaptcha/index.js Outdated

+                  error: undefined,
+                  onChange: () => {},
+                  className: undefined,
+                  style: {},

Member

LeandroTorresSicilia Oct 21, 2018

in all components we are setting style to undefined

LeandroTorresSicilia reviewed

View reviewed changes

src/components/ReCaptcha/loader.js Outdated

+              import RenderIf from '../RenderIf';
+              import './styles.css';
+              export default class ReCaptchaLoader extends Component {

Member

LeandroTorresSicilia Oct 21, 2018

it better call this ReCaptchaComponent and the file component.js

LeandroTorresSicilia reviewed

View reviewed changes

src/components/ReCaptcha/loader.js Outdated

+                          explicit: true,
+                          sitekey: value,
+                          theme,
+                          size,

Member

LeandroTorresSicilia Oct 21, 2018

pass tabIndex here

LeandroTorresSicilia reviewed

View reviewed changes

src/components/ReCaptcha/loader.js Outdated

+                      const { error, style } = this.props;
+                      return (
+                          <div className={this.getContainerClassNames()} style={style}>
+                              <div id="reCaptcha-container" />

Member

LeandroTorresSicilia Oct 21, 2018

id="recaptcha-container"

LeandroTorresSicilia reviewed

View reviewed changes

src/components/ReCaptcha/loader.js Outdated

+                      return (
+                          <div className={this.getContainerClassNames()} style={style}>
+                              <div id="reCaptcha-container" />
+                              <RenderIf isTrue={error}>

Member

LeandroTorresSicilia Oct 21, 2018

Review the console and you will see a warning because you are not passing a bool here your are passing undefined, a node or a string. So:

<RenderIf isTrue={!!error}>

LeandroTorresSicilia reviewed

View reviewed changes

src/components/ReCaptcha/loader.js Outdated

+              }
+              ReCaptchaLoader.propTypes = {
+                  /** Specifies the site key for the recaptcha. */

Member

LeandroTorresSicilia Oct 21, 2018

here you don't need to put description, they are only needed in the index.js

LeandroTorresSicilia reviewed

View reviewed changes

src/components/ReCaptcha/loader.js Outdated

+              ReCaptchaLoader.defaultProps = {
+                  value: undefined,
+                  theme: 'light',
+                  size: 'normal',

Member

LeandroTorresSicilia Oct 21, 2018

And you can put the props that receive a value different form undefined and null as required.
In this case theme, size, onChange can be set as required since they come from index with 'light', 'normal' and () => {}.

LeandroTorresSicilia reviewed

View reviewed changes

src/components/ReCaptcha/loader.js Outdated

+                  renderReCaptcha() {
+                      const { value, theme, size, onChange } = this.props;
+                      window.grecaptcha.render('reCaptcha-container', {
+                          explicit: true,

Member

LeandroTorresSicilia Oct 21, 2018

remove this


          fix: renamed loader.js to component.js, moved getUrl function to a file

8adbcad

codeclimate bot reviewed

View reviewed changes

src/components/ReCaptcha/component.js

+                  }
+                  renderReCaptcha() {
+                      const { value, theme, size, tabIndex, onChange } = this.props;

codeclimate bot Oct 21, 2018

Similar blocks of code found in 6 locations. Consider refactoring.

coveralls commented Oct 21, 2018 •

edited

Loading

Pull Request Test Coverage Report for Build 1481

0 of 0 changed or added relevant lines in 0 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-1.2%) to 72.376%

Totals
Change from base Build 1468:	-1.2%
Covered Lines:	927
Relevant Lines:	1218

💛 - Coveralls


          fixL removed default prop

a7be0f6

codeclimate bot commented Oct 21, 2018

Code Climate has analyzed commit a7be0f6 and detected 1 issue on this pull request.

Here's the issue category breakdown:

Category	Count
Duplication	1

View more on Code Climate.

LeandroTorresSicilia approved these changes

View reviewed changes

LeandroTorresSicilia merged commit 373e42c into master

reiniergs deleted the implement-recaptcha branch

October 21, 2018 05:51

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet