-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
[No QA] Use double negation instead of Boolean() #42492
Conversation
Hey! I see that you made changes to our Form component. Make sure to update the docs in FORMS.md accordingly. Cheers! |
@ishpaul777 Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
I assume there's no visual changes to this? |
@ShridharGoel Prettier check is failing, could you look into it? |
Will update this once eslint-config-expensify PR is merged. |
Will update this tomorrow. |
Correct |
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.
@ShridharGoel aren't you forgetting to apply the new lint rule to this PR?
That was merged yesterday only, I'll update this PR. |
ah, sorry I missed that detail. Going to mark this as WIP in the meantime. Feel free to remove the prefix when it's ready for review. Thanks! |
Waiting on #42650. |
@ShridharGoel #42650 was merged! Let's continue with this PR 😄 |
Updated. |
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.
Tested it in my IDE, works well but ESLint was working pretty slow for me. Could you benchmark ESLint before and after this rule was introduced?
@ishpaul777 This is not a functional change, do videos need to be added? |
yes, demo videos/screenshots are mandatory since here there isn't anything specific to record, just show that the basic functionalities like navigation, sending messages, submitting expense etc. work properly. |
Will add. |
@ShridharGoel You have conflicts! Also Can we please speed up here its long overdue |
Updating again.
…On Thu, Jun 6, 2024, 8:43 PM Ishpaul Singh ***@***.***> wrote:
@ShridharGoel <https://github.com/ShridharGoel> You have conflicts! Also
Can we please speed up here its long overdue
—
Reply to this email directly, view it on GitHub
<#42492 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AIPLJHCCHL4T4ITO544NAI3ZGB4BBAVCNFSM6AAAAABIEJ3QISVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNJSG44DKOBSGY>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
@ishpaul777 Updated. |
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! (just waiting for author checklist videos)
I had added 4 of them some hours back, was facing some issue with mweb. So, that is pending but will add them also. |
Oh, looks like it didn't get saved back then. @ishpaul777 Can you check now? Edit: Added mWeb ones also. |
@ishpaul777 Kindly ignore the duplicate review request. |
🚀 Deployed to production by https://github.com/luacmartins in version: 1.4.81-11 🚀
|
Details
Use double negation instead of Boolean().
Fixed Issues
$ #39126
PROPOSAL: #39126 (comment)
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.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
Screen.Recording.2024-06-06.at.9.27.39.PM.mov
Android: mWeb Chrome
Screenrecording_20240607_021553.mp4
iOS: Native
Screen.Recording.2024-06-06.at.9.23.54.PM.mov
iOS: mWeb Safari
Screen.Recording.2024-06-07.at.2.09.36.AM.mov
MacOS: Chrome / Safari
Test.mov
MacOS: Desktop
Screen.Recording.2024-06-06.at.9.19.55.PM.mov