-
Notifications
You must be signed in to change notification settings - Fork 137
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
remove extra space above codeblocks #441
remove extra space above codeblocks #441
Conversation
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
I have read the CLA Document and I hereby sign the CLA |
recheck |
@aneequeahmad changes look good! Here are some chores for ya
Thanks |
lib/ExpensiMark.js
Outdated
name: 'replacebr', | ||
regex: /<br\s*[\/]?><pre>\s*/gi, | ||
replacement: ' <pre>' | ||
} |
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.
Add a trailing comma
} | |
}, |
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.
Ahh yes, you're right, adding trailing comma now. Also, apologies for these mistakes this is my first PR in Expensify.
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.
It's no biggie 😄 Thanks for your quick response!
In future, you could just run npx eslint foldername --fix
to avoid this.
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.
Thanks for your suggestion @rushatgabhane. I would do it next time.
Updated the PR with feedback |
@aneequeahmad thanks! |
@rushatgabhane i forced pushed another commit where i added comment for added rule. When i was commiting it asked me for the key so i added it. Also adding GPG key in github is throwing error. |
@aneequeahmad we require all commits to be verified. If you're still having issues with that, then I'd suggest that you ask for help in
Thanks! |
It seems you managed to sign the commits! I suggest closing this PR and reopen a new one with all signed commits otherwise we can't merge it |
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.
@aneequeahmad just some comment suggestions.
@@ -163,11 +163,17 @@ export default class ExpensiMark { | |||
replacement: '<br />', | |||
}, | |||
{ | |||
// when <pre> and <br /> occur together extra line is added so removing <br /> | |||
// when </pre> and <br/> occur together extra line is added so removing <br /> |
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.
// when </pre> and <br/> occur together extra line is added so removing <br /> | |
// We're removing <br /> because when </pre> and <br /> occur together, an extra line is added. |
replacement: '</pre>', | ||
}, | ||
{ | ||
// when <br/> and <pre> occur together extra line is added so replacing with <pre> |
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.
// when <br/> and <pre> occur together extra line is added so replacing with <pre> | |
// We're removing <br /> because when <br /> and <pre> occur together, an extra line is added. |
Details
Fixed Issues
Expensify/App#8089
Tests
PR Review Checklist
Contributor (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 */
displayName
propertythis
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
### Fixed Issues
section aboveTests
sectionQA steps
sectiontoggleReport
and notonIconClick
).src/languages/*
filesSTYLE.md
) were followed/** comment above it */
displayName
propertythis
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)QA Steps
Screenshots
Web
Mobile Web
Desktop
iOS
Android
Unfortunately rn can't run on android simulator is broken :( Though i tested on real device when fixed but couldn't take screenshot at that time