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

Conditional Rendering translation #19

Merged

Conversation

ElRodrigote
Copy link
Contributor

@ElRodrigote ElRodrigote commented Feb 1, 2019

Related with #17

@ElRodrigote ElRodrigote changed the title Rodrigo Ali - First translation work, changes to create the PR WIP: Conditional Rendering translation Feb 1, 2019
@ElRodrigote
Copy link
Contributor Author

@alejandronanez I finished the translation, but don't know how to add people to this PR so they can review it.

@ElRodrigote ElRodrigote changed the title WIP: Conditional Rendering translation Conditional Rendering translation Feb 3, 2019
Copy link
Member

@alejandronanez alejandronanez left a comment

Choose a reason for hiding this comment

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

Hey @ElRodrigote, thanks for this PR.

I think we need to change a couple of things before merging it.

  1. You translated almost all the code, but right now it is completely different to the one in the codepen link. Please roll back it to English so everything is in sync.
  2. I see you added an $INLINE word in the translation, what is it for? I suggested some translation for it, but I'm not 100% if it satisfies what we want to communicate. We're open to suggestions about that $INLINE.

Let us know if you have any question,

Thanks!

content/docs/conditional-rendering.md Outdated Show resolved Hide resolved
content/docs/conditional-rendering.md Outdated Show resolved Hide resolved
content/docs/conditional-rendering.md Show resolved Hide resolved

### Inline If with Logical && Operator
### If $INLINE con operador lógico &&
Copy link
Member

Choose a reason for hiding this comment

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

What does this inline stand out for?

Copy link
Member

Choose a reason for hiding this comment

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

If we are not translating $INLINE, I think we could say en la misma línea or something similar.

Copy link

@gariasf gariasf Feb 4, 2019

Choose a reason for hiding this comment

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

That title refers to the use of things like {isVisible && <Component />}. That's an inline if operator, simpler and easier to read than traditional block based or ternary ones, specially useful inside of JSX expressions. Just FYI.

If isVisible is false, the statement will return false and therefore <Component /> won't render and vice-versa.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gotcha. Will take a look at that and translated snippet @123

);
```

[**Try it on CodePen**](https://codepen.io/gaearon/pen/ozJddz?editors=0010)
[**Pruébalo en CodePen**](https://codepen.io/gaearon/pen/ozJddz?editors=0010)
Copy link
Member

Choose a reason for hiding this comment

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

Please update the code in lines 127-147 to match this codepen link as much as possible. I think it's fine to change the content of the JSX but not the component names neither the root node.

content/docs/conditional-rendering.md Outdated Show resolved Hide resolved
@ElRodrigote
Copy link
Contributor Author

  1. You translated almost all the code, but right now it is completely different to the one in the codepen link. Please roll back it to English so everything is in sync.
    Yeah, that was one of my concerns, I'll revert theses changes ASAP!
  1. I see you added an $INLINE word in the translation, what is it for? I suggested some translation for it, but I'm not 100% if it satisfies what we want to communicate. We're open to suggestions about that $INLINE.
    I didn't know which translation would be correct for "inline". "En linea"? Any suggestion or other place where it was used?

Thanks for the review!

@ElRodrigote
Copy link
Contributor Author

@alejandronanez thanks for the CR, I think I've addressed all of them :)

@ElRodrigote
Copy link
Contributor Author

@gariasf Thanks for the CR, all the file was just reviewed, I think it's good to go now :), but please take a final look if you guys have the time to with @alejandronanez

Copy link
Member

@alejandronanez alejandronanez left a comment

Choose a reason for hiding this comment

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

Hey @ElRodrigote, almost there!

Please update your PR with the suggestions, all code should remain in English. Thanks for the hard work (and your patience) 💪 !

content/docs/conditional-rendering.md Outdated Show resolved Hide resolved
content/docs/conditional-rendering.md Outdated Show resolved Hide resolved
content/docs/conditional-rendering.md Outdated Show resolved Hide resolved
content/docs/conditional-rendering.md Outdated Show resolved Hide resolved
Copy link
Member

@alejandronanez alejandronanez left a comment

Choose a reason for hiding this comment

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

LGTM @ElRodrigote, thanks for being a contributor! 👍

Welcome, keep the PRs coming!

@alejandronanez alejandronanez merged commit 7bcb62d into reactjs:master Feb 8, 2019
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