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 ability to use value prop and fall back to children #53

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kborchers
Copy link
Collaborator

Fixes #34

@kborchers
Copy link
Collaborator Author

@rxaviers a review would be great.

Also, I wanted to add the ability to use value prop in the FormatMessage component for consistency in the react-globalize components but when writing a test for an empty string in the value prop, Globalize would throw Error: E_INVALID_MESSAGE: Invalid message content``. a string expected. I could think of a few instances where I would want an empty string for initialization or resetting some text. Is this not possible with Globalize or did I miss something? If not possible, maybe worth adding?

@rxaviers
Copy link
Member

Excellent, reviewing it is in my TODO list now.

About the empty message, technically it's an easy change, but I'm wondering why would you want to internationalize an empty string? Would it be different in other locales?

@kborchers
Copy link
Collaborator Author

About the empty message, technically it's an easy change, but I'm wondering why would you want to internationalize an empty string? Would it be different in other locales?

Definitely not different in other locales but because of the way react-globalize works, there's no way to say "don't try to internationalize this component until you have a path or children". Maybe it's up to the dev to completely remove the component and add it back in when a value is present but that seems bad. I'm not sure of the right answer here but thought I would bring it up.

To further illustrate, perhaps my app has an "Info Bar" that displays internationalized messages based on user interaction. It would be nice on initial load for the "Info Bar" component to maybe look something like

<InfoBar maybesomeotherprops><FormatMessage/></InfoBar>

or something like that but that throws Error: Invalid default message type undefined which could be an issue on the react-globalize side or Globalize or both, I'm not sure.

@rxaviers
Copy link
Member

Kris, just reviewed it.

The option to use either a value attribute or the children is a great addition in my opinion, but I vote to make them mutually exclusive, i.e., use one or another for two reasons: (a) while the option is great, I don't see a value of allowing both simultaneously canceling each other, (b) in case it's allowed simultaneously, it could be error prone to update the attribute in the children, but having the value canceling it; it might be hard to spot the problem. Therefore, I suggest we throw an Error in case both are set.

@rxaviers
Copy link
Member

About the empty message, I would defer this until there's a compelling use case. I believe one can simply leave it as <InfoBar maybesomeotherprops></InfoBar> with no much prejudice and if we decide to allow an empty message, we'll end up with an useless empty entry in the translation JSON file, which is pointless.

@necolas
Copy link
Contributor

necolas commented Dec 14, 2016

AFAICT, you can use path or children…and this also adds value?

@kborchers
Copy link
Collaborator Author

Thanks @necolas, I will try to look into this tonight. It's been quite a while since I've touched this project and there are likely many conflicts to resolve but I will take a stab at it.

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.

5 participants