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.

Sign up for GitHub

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

Jump to bottom

Avoid removing break before and after heading #531

Merged
merged 3 commits into from
May 13, 2023
Merged

Avoid removing break before and after heading #531

merged 3 commits into from
May 13, 2023

Conversation

eh2077
Copy link
Contributor

@eh2077 eh2077 commented May 9, 2023

Fixed Issues

$ Expensify/App#17998

Tests

  1. Add a comment
========= test case 2 ========
> [quote] case 2
> 
> # heading
> 
> [quote] case 2

[normal] case 2

# heading

[normal] case 2
  1. Verify that there's a line break before and after the heading for both normal header and header inside quote
  2. Edit the comment and verify that the markdown is same as the input.
Screen.Recording.2023-05-09.at.7.20.49.PM.mov

QA

Same as test

@eh2077 eh2077 requested a review from a team as a code owner May 9, 2023 12:42
@melvin-bot melvin-bot bot requested review from neil-marcellini and removed request for a team May 9, 2023 12:43
@eh2077 eh2077 mentioned this pull request May 9, 2023
Closed
6 tasks
eVoloshchak
eVoloshchak approved these changes May 10, 2023
View reviewed changes
Copy link
Contributor

@eVoloshchak eVoloshchak left a comment

Choose a reason for hiding this comment

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

Tests well

@eh2077
Copy link
Contributor Author

eh2077 commented May 11, 2023

@mountiny Friendly bump!

mountiny
mountiny approved these changes May 13, 2023
View reviewed changes
Copy link
Contributor

@mountiny mountiny left a comment

Choose a reason for hiding this comment

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

Thanks everyone, sorry I have been at conference

@mountiny mountiny merged commit e7e765d into Expensify:main May 13, 2023
@tienifr tienifr mentioned this pull request May 25, 2023
Merged
55 tasks
@eh2077 eh2077 mentioned this pull request Jun 30, 2023
Merged
55 tasks
@parasharrajat parasharrajat mentioned this pull request Sep 18, 2023
Closed
6 tasks
parasharrajat
parasharrajat reviewed Sep 20, 2023
View reviewed changes
lib/ExpensiMark.js
@@ -235,7 +235,7 @@ export default class ExpensiMark {
},
{
name: 'heading1',
regex: /\s*<(h1)(?:"[^"]*"|'[^']*'|[^'">])*>(.*?)<\/\1>(?![^<]*(<\/pre>|<\/code>))\s*/gi,
regex: /[^\S\r\n]*<(h1)(?:"[^"]*"|'[^']*'|[^'">])*>(.*?)<\/\1>(?![^<]*(<\/pre>|<\/code>))[^\S\r\n]*/gi,
Copy link
Member

Choose a reason for hiding this comment

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

I am curious to know why we are matching the newlines after the h1 tag. @eh2077 I don't see this clearly explained in proposal.

Copy link
Member

@parasharrajat parasharrajat Sep 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

any thoughts on how this has been working together with other pieces of the parser?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@parasharrajat Sorry for late reply. The App prefers to let user input show as it is especially for line breaks. So we don't consume the line breaks here. I'm not sure if I answered your question. You can link the related issues, so I can have better context for what you ask.

Copy link
Member

Choose a reason for hiding this comment

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

I didn't understand. Can you please summarize the problem and solution? I want to see how this logic fits into the whole picture. This way I can imagine the side-effect of changing something. #577 is the PR where we are changing this.

@parasharrajat parasharrajat mentioned this pull request Sep 26, 2023
Merged
parasharrajat
parasharrajat reviewed Oct 19, 2023
View reviewed changes
lib/ExpensiMark.js
@@ -235,7 +235,7 @@ export default class ExpensiMark {
},
{
name: 'heading1',
regex: /\s*<(h1)(?:"[^"]*"|'[^']*'|[^'">])*>(.*?)<\/\1>(?![^<]*(<\/pre>|<\/code>))\s*/gi,
regex: /[^\S\r\n]*<(h1)(?:"[^"]*"|'[^']*'|[^'">])*>(.*?)<\/\1>(?![^<]*(<\/pre>|<\/code>))[^\S\r\n]*/gi,
Copy link
Member

Choose a reason for hiding this comment

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

This change caused Expensify/App#27445.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@parasharrajat parasharrajat parasharrajat left review comments

@eVoloshchak eVoloshchak eVoloshchak approved these changes

@mountiny mountiny mountiny approved these changes

@neil-marcellini neil-marcellini Awaiting requested review from neil-marcellini

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@eh2077 @eVoloshchak @parasharrajat @mountiny