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 renderNullWhileWaiting option #400

Merged
merged 5 commits into from Mar 8, 2018
Merged

Add renderNullWhileWaiting option #400

merged 5 commits into from Mar 8, 2018

Conversation

@perrin4869
Copy link
Contributor

perrin4869 commented Mar 6, 2018

Only would need to document the new option - if waiting is set to true, and we are not rendering null, we pass a ready flag to the children of I18next. If we're using the HoC, then in order to prevent naming clashes, rename ready as tReady and pass it as a prop to the WrappedComponent

@jamuhl

This comment has been minimized.

Copy link
Member

jamuhl commented Mar 6, 2018

Will review and merge tomorrow - and update the documentation for the new options.

Thank you for taking the time to provide the PR.

@perrin4869

This comment has been minimized.

Copy link
Contributor Author

perrin4869 commented Mar 6, 2018

By the way, eslint is complaining about unused state in https://github.com/i18next/react-i18next/pull/400/files#diff-686338533c52a0b5b74b17d3c0e73e9dR104 - any reason it is there?

@jamuhl

This comment has been minimized.

Copy link
Member

jamuhl commented Mar 6, 2018

hm...we used to pass that down to the component as prop to trigger rerender -> seems not to be needed anylonger as we changed to use render props...but will need to double check

@tkvw

This comment has been minimized.

Copy link

tkvw commented Mar 6, 2018

Nice, I actually have the exact same problem at the moment. I think the ready prop should be passed down when waiting is false (without setting an option for it). My issue is that I have an additional rerender on initial load, because in my setting waiting is false and I listen for languageChanged event only, but the state will change when ready state is set. I can create a SCU for this, but only if I receive the prop.

So can we pass it through without a special renderNullWhileWaiting prop? I think this adds extra unnecessary complexity to the config.

@jamuhl

This comment has been minimized.

Copy link
Member

jamuhl commented Mar 6, 2018

@tkvw means we could simplify

https://github.com/i18next/react-i18next/pull/400/files#diff-686338533c52a0b5b74b17d3c0e73e9dR128

and

https://github.com/i18next/react-i18next/pull/400/files#diff-92a48300b7f85dd6b170af70986e962eR72

to include that in every case. As ready is already set to true on constructing https://github.com/i18next/react-i18next/blob/master/src/I18n.js#L33 i think this should be save.

@perrin4869 what do you think - so we always pass down ready...just avoid returning null in case that renderNullWhileWaiting is set to false.

@tkvw

This comment has been minimized.

Copy link

tkvw commented Mar 6, 2018

@jamuhl : Thanks for pointing out https://github.com/i18next/react-i18next/blob/master/src/I18n.js#L33 , I now make sure languages and the resourcebundles of the default locale are setup correctly so ready will be true on initial render. This solves my problem.

@perrin4869

This comment has been minimized.

Copy link
Contributor Author

perrin4869 commented Mar 7, 2018

OK I will change it so the ready flag is passed regardless of waiting or not!

@perrin4869

This comment has been minimized.

Copy link
Contributor Author

perrin4869 commented Mar 7, 2018

That would make the renderNullWhileWaiting option obsolete btw

@jamuhl

This comment has been minimized.

Copy link
Member

jamuhl commented Mar 7, 2018

@perrin4869 no. if both wait and renderNullWhileWaiting are set true it will return null. This new case only applies if wait is set to false.

@perrin4869

This comment has been minimized.

Copy link
Contributor Author

perrin4869 commented Mar 7, 2018

Hm... aren't wait and renderNullWhileWaiting equivalent? What's the difference?
What do you do when wait is true and renderNullWhileWaiting is false?

@jamuhl

This comment has been minimized.

Copy link
Member

jamuhl commented Mar 7, 2018

wait: true , ready: false, renderNullWhileWaiting: true -> returns null
wait: true , ready: false, renderNullWhileWaiting: false -> returns component with ready prop false
wait: false, ready: false, renderNullWhileWaiting: true -> returns component with ready prop false

@jamuhl

This comment has been minimized.

Copy link
Member

jamuhl commented Mar 7, 2018

honestly we can remove renderNullWhileWaiting ;)

just set wait: false and check for ready prop

@jamuhl

This comment has been minimized.

Copy link
Member

jamuhl commented Mar 7, 2018

i think that is even the best solution -> you do not want to wait but you get that ready false to react on

@coveralls

This comment has been minimized.

Copy link

coveralls commented Mar 7, 2018

Coverage Status

Coverage decreased (-0.08%) to 81.405% when pulling 36f0400 on perrin4869:ready into 36dad75 on i18next:master.

@perrin4869

This comment has been minimized.

Copy link
Contributor Author

perrin4869 commented Mar 7, 2018

Done! Things should be in order now :)
Just update docs to include the new props/args ready and tReady and ideally write some tests - I'm not acquainted enough with i18next internals to come up with tests promptly.
But it's a straightforward passing down of props so testing should not be critical here

@jamuhl

This comment has been minimized.

Copy link
Member

jamuhl commented Mar 7, 2018

will check this tomorrow and merge, publish and update docs where needed

@jamuhl jamuhl merged commit 7d35821 into i18next:master Mar 8, 2018
2 of 3 checks passed
2 of 3 checks passed
coverage/coveralls Coverage decreased (-0.08%) to 81.405%
Details
codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jamuhl

This comment has been minimized.

Copy link
Member

jamuhl commented Mar 8, 2018

published in react-i18next@7.5.0

will update docs in a few minutes / hours (rather busy right now with other stuff)

@jamuhl

This comment has been minimized.

Copy link
Member

jamuhl commented Mar 8, 2018

Thanks to all involved making this happen.

@perrin4869

This comment has been minimized.

Copy link
Contributor Author

perrin4869 commented Mar 9, 2018

Thanks for maintaining this as well as always @jamuhl :)

@ChristiaanScheermeijer

This comment has been minimized.

Copy link

ChristiaanScheermeijer commented Jul 13, 2018

Hi guys, is there an option to disable this behavior? After updating I need to update all translated components which are using {...rest} to inherit props.

@jamuhl

This comment has been minimized.

Copy link
Member

jamuhl commented Jul 13, 2018

@ChristiaanScheermeijer can you make a sample - just not get the problem by current description - as i'm aware of there should be no breaking change

@ChristiaanScheermeijer

This comment has been minimized.

Copy link

ChristiaanScheermeijer commented Jul 13, 2018

Hey @jamuhl, sure!

I have multiple components which are using this pattern:

class LoginButton extends Component {
  render() {
    const { t, ...rest } = this.props;

    return (
      <Button component={Link} to="/login" {...rest}>
        {t('button')}
      </Button>
    );
  }
}

export default providers(
  LoginButton,
  translate('login'),
);

Since now the Button component is receiving an unknown tReady prop, I get the following warning:

Warning: React does not recognize the tReady prop on a DOM element.

@jamuhl

This comment has been minimized.

Copy link
Member

jamuhl commented Jul 13, 2018

@ChristiaanScheermeijer i see...i guess there is not much we can do...you will have to change your code i guess -> i would suggest as best practice never pass down ...rest to DOM elements -> use something like https://lodash.com/docs/4.17.10#pick

@ChristiaanScheermeijer

This comment has been minimized.

Copy link

ChristiaanScheermeijer commented Jul 13, 2018

Ok, I will use this workaround for now:

export const providers = (component, ...wrappers) => {
  let WrappedComponent = component;

  wrappers.forEach(fn => WrappedComponent = fn(WrappedComponent));

  return ({ tReady, ...props}) => <WrappedComponent {...props} />;
};

The ...rest props are being passed down to the material-ui/Button component. I use this to prevent having this multiple times 😅 :

<Button
  component={this.props.component}
  to={this.props.to}
  color={this.props.color}
  disabled={this.props.disabled}
  disableFocusRipple={this.props.disableFocusRipple}
  disableRipple={this.props.disableRipple}
  fullWidth={this.props.fullWidth}
  href={this.props.href}
  mini={this.props.mini}
  size={this.props.size}
  variant={this.props.variant}
  action={this.props.action}
  buttonRef={this.props.buttonRef}
  centerRipple={this.props.centerRipple}
  type={this.props.type}
  {...evenMoreProps}
/>

Thanks for looking into this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.