-
Notifications
You must be signed in to change notification settings - Fork 73
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
Revert tab leader PRs #458
Revert tab leader PRs #458
Conversation
@chrispader was this a clean revert for both PRs? |
no, there were conflicts due to the later PRs. I'm currently checking if anything got messed up |
@mountiny this PR should be ready now. Accidentally removed a call to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@chrispader looking good, I think we can keep the screenshots in the App Pr when testing the Onyx bump since this is just a revert
@NikkiWines all yours
Great! added screenshots and tested on web, iOS and Android anyway. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@koko57 @roryabraham @mountiny
Details
Reverts #382 and #434 so that we can bump Onyx in E/App.
Once the issues with this functionality are resolved, we can re-apply these PRs
Related Issues
#455 (comment)
Automated Tests
Manual Tests
Author Checklist
### Related Issues
section aboveTests
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
this
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop
Tested on web, iOS and Android.