Pr global legal

[supra08]

No description provided.

[dashwave]

Build Status	✅ Successful
Build Time	22s
📱 Emulation Link	https://console.dashwave.io/emulator/1706/preview?device=emulator
Logs	See Logs

[supra08]

@dashwave /review

[dashwave]

Pull Request Description

Title: LegalScreen: Turn into a global, all-accounts screen

What is this?

This pull request introduces changes to the LegalScreen component to make it a global, all-accounts screen, ensuring that users are aware of the legal terms associated with all realms the app interacts with, not just the active account's realm. This approach simplifies user interaction by aggregating all relevant terms on one page, paving the way for enhanced multi-account support in the future.

Changes

Added Features:

1. New Selector in src/settings/LegalScreen.js:
   • getViewModel: Aggregates data from all realms, consisting of the realm URL, name, and policies URL, even if the user is not logged in.

Code Changes:

1. In src/nav/AppNavigator.js:

   • Moved LegalScreen outside the useHaveServerDataGate wrapper to show terms for all accounts, not just the active account.

2. In src/settings/LegalScreen.js:

   • Created a new ViewModel type to represent data for all realms.
   • Introduced a new selector getViewModel to compute the aggregated data.
   • Updated the LegalScreen component to use the ViewModel and iterate over all realms to show their terms.

Documentation Updates:

1. In src/settings/LegalScreen.js:
   • Added detailed comments explaining the rationale behind the data aggregation.
   • Documented the ViewModel type and the new selector getViewModel.

Minor Changes:

1. In src/settings/LegalScreen.js:
   • Introduced a constant, zulipPoliciesUrl, to store the Zulip policies URL.
   • Updated the "Zulip terms" line to include the policy URL for consistency.

---

Pull Request Review

Code Review

The changes introduced in this pull request are quite significant and generally well-executed. Here's a detailed review:

1. Cleanliness and Readability:

   • The code is clean and well-structured. The logic for aggregating terms across all realms is clearly laid out.
   • The use of the ViewModel type and the getViewModel selector is straightforward and easy to understand.

2. Integration with Existing Codebase:

   • The integration of the new ViewModel within the LegalScreen component is seamless.
   • The change in AppNavigator.js to move the LegalScreen outside the useHaveServerDataGate wrapper appears to be a thoughtful decision, ensuring the screen is accessible regardless of active account status.

3. Documentation and Comments:

   • Comprehensive comments and documentation have been added, explaining the intention behind the changes and how the new logic works.
   • The additional detail provided in the comments will be beneficial for future maintenance and understanding.

Suggestions

1. Error Handling:

   • While the selector is robust, it might be worth adding error handling for cases where the data might not be available or URL parsing fails.

2. Performance Considerations:

   • If the number of realms is expected to grow considerably, ensure the selector getViewModel is optimized for performance to avoid sluggish UI updates.

Conclusion

Overall, this is a high-quality pull request that enhances the LegalScreen component significantly. It adheres to best practices, and the added documentation ensures that future developers will easily understand the changes. Excellent work!