-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Update expensify-common #11996
Update expensify-common #11996
Conversation
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
I have read the CLA Document and I hereby sign the CLA |
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.
LGTM!
@cristipaval I am trying to test this. As soon as I send the Screen.Recording.2022-10-20.at.8.12.22.PM.mov |
@mananjadhav that's strange..it's working as expected for me. |
Ohh! I checked with @cristipaval. This is because the change on the backend is yet to be deployed. Can you update here @cristipaval once it is deployed and I can take a look at it again? |
I think we can leave that for QA! @cristipaval what do you think? |
PR Reviewer Checklist
|
@youssef-lr , I didn't realise that this PR won't work without the web PR being deployed. It worked on your local machine because you tested against dev. @mananjadhav tested against staging and the web pr is not in staging yet. My suggestion is to mark this PR on hold until the web PR gets deployed. So even if you both approve it, I can't merge it before web changes are deployed. |
Sounds good to me. |
@cristipaval I found one issue while testing the PR. I saw this is deployed to staging, so I am connecting to the staging backend and testing it. Android H1 with long text |
Hey @mananjadhav I can't reproduce this on my Android device. Is it always reproducible on your end? What Android version you run and on what mobile phone? |
Hi @cristipaval I am using an Android emulator to test this. I used Pixel 5 Android emulator. |
|
Added |
@cristipaval I think we can remove the hold and merge this. |
Thanks @mananjadhav and @youssef-lr ! I tested with real Samsung Galaxy S20 and Samsung Galaxy A03 and I couldn't reproduce it. I'm discussing with the team atm to decide if we merge it as it is for now or we want more investigation. |
@cristipaval looks like this was merged without the Run e2e performance regression tests test passing. Please add a note explaining why this was done and remove the |
Merged because this is blocking #churnedcustomers_engaged25 |
@cristipaval Just saw this was merged. I tested this on a real device - OnePlus 6, and I can still reproduce the issue. Guessing we would now want to fix this with a follow up PR? |
Update expensify-common (cherry picked from commit 883f84f)
…-11996 🍒 Cherry pick PR #11996 to staging 🍒
With further testing it turns out if the text didn't have the link it worked fine. If I added the link, it cuts off. |
🚀 Cherry-picked to staging by @cristipaval in version: 1.2.19-2 🚀
@Expensify/applauseleads please QA this PR and check it off on the deploy checklist if it passes. |
🚀 Deployed to production by @chiragsalian in version: 1.2.19-2 🚀
|
🚀 Cherry-picked to staging by https://github.com/AndrewGable in version: 1.3.28-2 🚀
@Expensify/applauseleads please QA this PR and check it off on the deploy checklist if it passes. |
🚀 Deployed to production by https://github.com/AndrewGable in version: 1.3.28-5 🚀
|
This wasn't an emergency, just a dependency bump. |
Details
Updates the expensify-common version to include markdown for heading1.
Fixed Issues
https://github.com/Expensify/Expensify/issues/225615
Tests
QA Steps
QA for regressions
PR Review Checklist
PR Author Checklist
### Fixed Issues
section aboveTests
sectionQA steps
sectiontoggleReport
and notonIconClick
)src/languages/*
filesSTYLE.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
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)PR Reviewer Checklist
The reviewer will copy/paste it into a new comment and complete it after the author checklist is completed
### Fixed Issues
section aboveTests
sectionQA steps
sectiontoggleReport
and notonIconClick
).src/languages/*
filesSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
have been tested & I retested again)/** 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
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Screenshots
Web
Mobile Web - Chrome
Mobile Web - Safari
Desktop
iOS
Android