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

fix: remove return null check from OverlayProvider and Chat component #2039

Merged
merged 7 commits into from
Apr 27, 2023

Conversation

khushal87
Copy link
Member

🎯 Goal

Fixes #2037

🛠 Implementation details

🎨 UI Changes

iOS
Before After
Android
Before After

🧪 Testing

☑️ Checklist

  • I have signed the Stream CLA (required)
  • PR targets the develop branch
  • Documentation is updated
  • New code is tested in main example apps, including all possible scenarios
    • SampleApp iOS and Android
    • Expo iOS and Android

@santhoshvai
Copy link
Member

santhoshvai commented Apr 4, 2023

Also, I think it is a good idea to remove some parts from useEffect so that it happens during render and not after the render maybe using usememo or bringing the translators state inside.. this whole hook seems badly done in the first place in my view 😢

I am talking about the following parts

    let streami18n: Streami18n;

    if (i18nInstance instanceof Streami18n) {
      streami18n = i18nInstance;
    } else {
      streami18n = new Streami18n({ language: 'en' });
    }

     streami18n.getTranslators().then((translator) => {
      if (translator && isMounted.current) setTranslators(translator);
    });

There is no reason to run the promise after the render actually if you think about it.. What say?

@khushal87
Copy link
Member Author

I have moved the translator state inside, because it makes sense to me.

@sonarcloud
Copy link

sonarcloud bot commented Apr 5, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

100.0% 100.0% Coverage
0.0% 0.0% Duplication

@@ -226,8 +217,6 @@ const ChatWithContext = <
enableOfflineSupport,
});

if (loadingTranslators) return null;
Copy link
Contributor

@vishalnarkhede vishalnarkhede Apr 12, 2023

Choose a reason for hiding this comment

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

Make sure to test it by adding some delays to streami18n.getTranslators() method. Just to check whats the behavior when translators is not ready

@vanGalilea vanGalilea merged commit c3ceb8c into develop Apr 27, 2023
@vanGalilea vanGalilea deleted the khushal87/i18n-null-check branch April 27, 2023 13:28
@github-actions github-actions bot mentioned this pull request Apr 28, 2023
6 tasks
@stream-ci-bot
Copy link
Contributor

🎉 This PR is included in version 5.14.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

First Render Always Blank
5 participants