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

feat(suite-native): offline header shows that app is offline #13501

Merged
merged 1 commit into from
Aug 6, 2024

Conversation

vytick
Copy link
Contributor

@vytick vytick commented Jul 24, 2024

Shows offline header stripe on top of app (after onboarding).

Related Issue

Resolve #13597

Screenshots:

@vytick vytick added the mobile Suite Lite issues and PRs label Jul 24, 2024
@vytick vytick requested a review from a team as a code owner July 24, 2024 19:42
Copy link

socket-security bot commented Jul 24, 2024

🚨 Potential security issues detected. Learn more about Socket for GitHub ↗︎

To accept the risk, merge this PR and you will not be notified again.

Alert Package NoteSourceCI
Network access npm/@react-native-community/[email protected] 🚫

View full report↗︎

Next steps

What is network access?

This module accesses the network.

Packages should remove all network access that is functionally unnecessary. Consumers should audit network access to ensure legitimate use.

Take a deeper look at the dependency

Take a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at support [AT] socket [DOT] dev.

Remove the package

If you happen to install a dependency that Socket reports as Known Malware you should immediately remove it and select a different dependency. For other alert types, you may may wish to investigate alternative packages or consider if there are other ways to mitigate the specific risk posed by the dependency.

Mark a package as acceptable risk

To ignore an alert, reply with a comment starting with @SocketSecurity ignore followed by a space separated list of ecosystem/package-name@version specifiers. e.g. @SocketSecurity ignore npm/[email protected] or ignore all packages with @SocketSecurity ignore-all

Copy link
Member

@matejkriz matejkriz left a comment

Choose a reason for hiding this comment

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

Code looks good, I'll test it. Message-system banners should stay visible even during onboarding, I believe. At least we should discuss that separately from offline banner.

I would like to double check if reachabilityUrl is ever used. In that case we should define our own, I guess.

suite-native/app/src/App.tsx Outdated Show resolved Hide resolved
suite-native/app/src/App.tsx Outdated Show resolved Hide resolved
@matejkriz
Copy link
Member

matejkriz commented Jul 30, 2024

Note for QA @STew790 : let's retest #10640 together with this?

@vytick
Copy link
Contributor Author

vytick commented Aug 1, 2024

/rebase

Copy link

github-actions bot commented Aug 1, 2024

@trezor-ci trezor-ci force-pushed the feat/native/offline-state-ui branch from bbcb365 to 9224e61 Compare August 1, 2024 20:32
@vytick vytick force-pushed the feat/native/offline-state-ui branch 2 times, most recently from 972aa8d to 2e99f94 Compare August 2, 2024 13:54
@PeKne
Copy link
Contributor

PeKne commented Aug 5, 2024

Screenshot 2024-08-05 at 13 37 41

On Android the status bar has unmatching color (it shoud be yellow, not white). Please change it via the https://reactnative.dev/docs/statusbar API.

suite-native/connection-status/src/OfflineHeader.tsx Outdated Show resolved Hide resolved

// If offline header is visible, return 0 for top inset, otherwise return the top inset from safe area insets
// this is beacuse the offline header is displayed on top of the screen and we don't want to add any top padding
export const useOfflineHeaderAwareSafeAreaInsets = () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm just wondering why this is needed. The MessageSystemBannerRenderer works the same way as the OfflineHeader component but a special "topAreInsetHook" is not required.

How is it different here? I would prefer not to override the default useSafeAreaInsets if possible for better DX.

Copy link
Contributor

@PeKne PeKne Aug 5, 2024

Choose a reason for hiding this comment

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

What also bugs me is that now all the packages imported to the suite-native/connection-status are now imported to all the places where are top area insets used. That could be quite troublesome in the future if this package will grow. Can imagine some nasty cyclical dependencies.

Copy link
Contributor Author

@vytick vytick Aug 5, 2024

Choose a reason for hiding this comment

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

this conditional inset is needed otherwise if the offlineheader is visible everything with usesafeareainset has odd big offset from the top.

Any idea how to do it differently is welcomed. 🙏

Copy link
Contributor

Choose a reason for hiding this comment

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

note: out of the scope of this PR

new issue created: #13659

@PeKne
Copy link
Contributor

PeKne commented Aug 5, 2024

Otherwise, it looks very good. Good job ✌️. Looking forward merging this.

@vytick
Copy link
Contributor Author

vytick commented Aug 5, 2024

On Android the status bar has unmatching color (it shoud be yellow, not white). Please change it via the https://reactnative.dev/docs/statusbar API.

<Screen> resets any background color change, so we agreed with matej to keep it this way for now

@vytick vytick force-pushed the feat/native/offline-state-ui branch from 1879ac4 to afb5cb2 Compare August 5, 2024 15:56
@vytick
Copy link
Contributor Author

vytick commented Aug 6, 2024

/rebase

Copy link

github-actions bot commented Aug 6, 2024

@trezor-ci trezor-ci force-pushed the feat/native/offline-state-ui branch from 24861cb to 1fddfe3 Compare August 6, 2024 10:45
@vytick
Copy link
Contributor Author

vytick commented Aug 6, 2024

@SocketSecurity ignore npm/@react-native-community/[email protected].

@vytick vytick force-pushed the feat/native/offline-state-ui branch 2 times, most recently from 55179bd to deaa12f Compare August 6, 2024 11:19
@vytick
Copy link
Contributor Author

vytick commented Aug 6, 2024

/rebase

Copy link

github-actions bot commented Aug 6, 2024

@trezor-ci trezor-ci force-pushed the feat/native/offline-state-ui branch from deaa12f to b6a4c39 Compare August 6, 2024 12:39
@vytick
Copy link
Contributor Author

vytick commented Aug 6, 2024

/rebase

Copy link

github-actions bot commented Aug 6, 2024

@trezor-ci trezor-ci force-pushed the feat/native/offline-state-ui branch from b6a4c39 to 6c49281 Compare August 6, 2024 14:12
@vytick vytick merged commit 1c6c918 into develop Aug 6, 2024
85 of 87 checks passed
@vytick vytick deleted the feat/native/offline-state-ui branch August 6, 2024 18:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
mobile Suite Lite issues and PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Mobile offline state
3 participants