Skip to content
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

Changes in quotes #385

Merged
merged 10 commits into from
Jun 24, 2021
Merged

Changes in quotes #385

merged 10 commits into from
Jun 24, 2021

Conversation

mateusbra
Copy link
Contributor

Created a process to quotes like what is done with modifyTextForUrlLinks at ExpensiMark module having a function to deal with quotes:

Fixed Issues

Expensify/App#2670

Tests

  1. Tested in the chat with these inputs:
> This is a one line quote

> This is a two line quote
> And this is the second line

> This is a three line quote
>
> With an empty line in the middle that has no space after the chevron

> This is another three line quote
>  
> But this time there's a space after the middle chevron
> This is another three-line quote
> 
>
>
> But this time there's a space after the middle chevron

QA

  1. Type in the keyboard and see that the quote now works correctly

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Android

print1
print2

@mateusbra mateusbra requested a review from a team as a code owner June 4, 2021 18:50
@github-actions
Copy link

github-actions bot commented Jun 4, 2021

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@MelvinBot MelvinBot requested review from TomatoToaster and removed request for a team June 4, 2021 18:50
@mateusbra
Copy link
Contributor Author

I have read the CLA Document and I hereby sign the CLA

lib/ExpensiMark.js Outdated Show resolved Hide resolved
lib/ExpensiMark.js Outdated Show resolved Hide resolved
@mateusbra
Copy link
Contributor Author

Changes were made in order to solve the tests and lints problems

Copy link
Contributor

@TomatoToaster TomatoToaster left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Had just one minor change I noticed. I'm going to tag someone else for review here too.

lib/ExpensiMark.js Outdated Show resolved Hide resolved
lib/ExpensiMark.js Outdated Show resolved Hide resolved
Copy link
Contributor

@roryabraham roryabraham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So in addition to the changes I've outlined so far, please add several unit tests in ExpensiMark-test.js to cover your changes.

Also, I really think you should separate the matching step from the replacement step, (i.e: follow the example here).

lib/ExpensiMark.js Outdated Show resolved Hide resolved
lib/ExpensiMark.js Outdated Show resolved Hide resolved
lib/ExpensiMark.js Outdated Show resolved Hide resolved
lib/ExpensiMark.js Outdated Show resolved Hide resolved
lib/ExpensiMark.js Outdated Show resolved Hide resolved
lib/ExpensiMark.js Outdated Show resolved Hide resolved
lib/ExpensiMark.js Outdated Show resolved Hide resolved
lib/ExpensiMark.js Outdated Show resolved Hide resolved
lib/ExpensiMark.js Outdated Show resolved Hide resolved
lib/ExpensiMark.js Outdated Show resolved Hide resolved
@mateusbra
Copy link
Contributor Author

So in addition to the changes I've outlined so far, please add several unit tests in ExpensiMark-test.js to cover your changes.

Also, I really think you should separate the matching step from the replacement step, (i.e: follow the example here).

Now the main part of quote replacement was moved to a replacement function as you said

@mateusbra
Copy link
Contributor Author

So in addition to the changes I've outlined so far, please add several unit tests in ExpensiMark-test.js to cover your changes.

Also, I really think you should separate the matching step from the replacement step, (i.e: follow the example here).

some tests were added for verifying quotes inside codefence

@roryabraham
Copy link
Contributor

FYI there are conflicts in this PR.

@mateusbra
Copy link
Contributor Author

FYI there are conflicts in this PR.

solved

lib/ExpensiMark.js Outdated Show resolved Hide resolved
lib/ExpensiMark.js Show resolved Hide resolved
lib/ExpensiMark.js Outdated Show resolved Hide resolved
lib/ExpensiMark.js Outdated Show resolved Hide resolved
roryabraham
roryabraham previously approved these changes Jun 23, 2021
Copy link
Contributor

@roryabraham roryabraham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is looking much better now.

TomatoToaster
TomatoToaster previously approved these changes Jun 23, 2021
Copy link
Contributor

@TomatoToaster TomatoToaster left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for resolving all the changes!

@TomatoToaster
Copy link
Contributor

@mateusbra, unfortunately we can't merge your PR because your commits are unsigned, for more details see here.

After setting up your commits to be signed, the simplest thing you can do is create a new branch and cherry pick the commits from this branch to there. Alternatively you can use git rebase and squish the commits to one and force push it, but I would not recommend testing that out in a seperate branch first so you don't lose your changes here.

@mateusbra mateusbra dismissed stale reviews from TomatoToaster and roryabraham via 1aff60e June 24, 2021 02:48
@mateusbra
Copy link
Contributor Author

@mateusbra, unfortunately we can't merge your PR because your commits are unsigned, for more details see here.

After setting up your commits to be signed, the simplest thing you can do is create a new branch and cherry pick the commits from this branch to there. Alternatively you can use git rebase and squish the commits to one and force push it, but I would not recommend testing that out in a seperate branch first so you don't lose your changes here.

@TomatoToaster thanks for helping me out, I had some problems with git cherry-pick (since the files were the same I couldn't make it without changing something), so I've added an apostrophe.

@TomatoToaster @roryabraham sorry for the mistake, I think you will have to approve again.

roryabraham
roryabraham previously approved these changes Jun 24, 2021
@roryabraham
Copy link
Contributor

@mateusbra This is still unmergable:

image

Here's a resource with instructions for setting up commit signing (it's not obvious to me which commits, if any, are signed): https://docs.github.com/en/github/authenticating-to-github/managing-commit-signature-verification/signing-commits

Once you've got that working so that any new commits are automatically signed, you've got several options:

  1. Create a new branch off main, and then cherry-pick the commits from this branch into that one.
  2. Create a new branch off main and then manually duplicate the changes from this branch in that one.
  3. Squash the commits in this branch into one signed commit and then force-push that commit to this branch. Instructions here.

@mateusbra
Copy link
Contributor Author

@roryabraham @TomatoToaster thanks for helping me out again, and sorry for the problems.
I think now its mergable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants