Feature/hide browser behind feature flag #20022 #20082
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Parveshdhull
approved these changes
May 17, 2024
Jenkins BuildsClick to see older builds (4)
|
90% of end-end tests have passedFailed tests (3)Click to expandClass TestWalletMultipleDevice:
Class TestGroupChatMultipleDeviceMergedNewUI:
Expected to fail tests (2)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityOneDeviceMerged:
Passed tests (47)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestDeepLinksOneDevice:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityMultipleDevicePRTwo:
Class TestWalletOneDevice:
Class TestActivityMultipleDevicePR:
Class TestCommunityOneDeviceMerged:
|
|
@flexsurfer FILE.2024-05-20.14.34.11.mp4 |
|
hey @churik for some reason SHOW_NOT_IMPLEMENTED_FEATURES is enabled in PRs, i'll disable it |
88% of end-end tests have passedFailed tests (4)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestWalletMultipleDevice:
Expected to fail tests (2)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityOneDeviceMerged:
Passed tests (46)Click to expandClass TestWalletOneDevice:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestActivityMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityMultipleDevicePRTwo:
Class TestCommunityMultipleDeviceMerged:
Class TestCommunityOneDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestDeepLinksOneDevice:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestActivityCenterContactRequestMultipleDevicePR:
|
flexsurfer
force-pushed
the
feature/Hide_browser_behind_feature_flag_#20022
branch
from
cc369d7
to
8bc23c5
Compare
|
hey @churik should be hidden in PRs now, thank you |
PR_ISSUE 1: Can't pin message in 1-1 chatSteps:
Expected result: Actual result: OS: IOS, Android |
PR_ISSUE 2: No option to enable browserI'd expect feature to ba available here |
thank @churik im sure it's not related to this PR, works fine for me, so probably some specific steps which can be replicated in nightly as well |
thank you @churik could you please elaborate on why you would expect it there? it's not under development, it's just not implemented feature, so it's under not implemented global flag, when we start working on it , it will be available in the feature flags |
79% of end-end tests have passedFailed tests (9)Click to expandClass TestWalletMultipleDevice:
Class TestCommunityMultipleDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestWalletOneDevice:
Expected to fail tests (2)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityOneDeviceMerged:
Passed tests (41)Click to expandClass TestActivityMultipleDevicePRTwo:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityOneDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestWalletOneDevice:
Class TestDeepLinksOneDevice:
|
PR_ISSUE 3: tapping on link on IOS leads to crashSteps:
Note: default browser on device is Chrome (it is not default) Expected result: no crash Actual result: FILE.2024-05-22.12.01.19.mp4OS: IOS |
|
thanks @churik fixed |
81% of end-end tests have passedFailed tests (8)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestWalletMultipleDevice:
Class TestWalletOneDevice:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityOneDeviceMerged:
Expected to fail tests (2)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityOneDeviceMerged:
Passed tests (42)Click to expandClass TestCommunityMultipleDeviceMergedTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityMultipleDevicePR:
Class TestDeepLinksOneDevice:
Class TestWalletOneDevice:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityMultipleDevicePRTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityOneDeviceMerged:
|
|
@Francesca-G for now there will be only 3 tabs, and for the next release we'll implement changes mentioned here |
ilmotta
reviewed
May 24, 2024
| (testing "dapps permissions are initialized" | ||
| (is (zero? (count (get-in cofx [:db :dapps/permissions])))) | ||
| (is (= dapp-id (get-in cofx [:db :browser/options :browser-id])))) | ||
| #_(deftest permissions-test |
There was a problem hiding this comment.
@flexsurfer, in the offsite there were discussions about the minimum number of approvals. We're striving for at least two approvals if possible and to always add more than just one reviewer. In this PR there was only one reviewer, so I kindly ask you to add more reviewers in future PRs. Here you can find the notes, but it's still not finished https://www.notion.so/Discussions-in-2024-offsite-b437526eecda4fdb999c0867abd080b6
cc @cammellos, @ulisesmac, @Parveshdhull
Doing a late review :)
About commenting out the tests like this, it would've been better to use the flag legacy.status-im.browser.core-test since that's what we're using already in this PR. It makes the intention in the code much clearer, whereas commented tests like this and in legacy.status-im.browser.core-test simply look like garbage/leftover.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
fixes #20022
we decided to go with that solution, hiding the browser button from tabs until we have a solution from the design team