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

Fix duplicated content and indentation in hide-markdown-diff #4052

Merged
merged 11 commits into from Mar 10, 2021
Merged

Fix duplicated content and indentation in hide-markdown-diff #4052

merged 11 commits into from Mar 10, 2021

Conversation

cheap-glitch
Copy link
Member

@cheap-glitch cheap-glitch commented Mar 3, 2021

Fixes #4035

Test URLs

Example: https://github.com/sindresorhus/refined-github/edit/main/readme.md

For a more thorough testing:

Create a Markdown file with the following content:
paragraph

paragraph

paragraph [REMOVED]

# level 1 heading
# level 1 heading
# level 1 heading [REMOVED]

## level 2 heading
## level 2 heading
## level 2 heading [REMOVED]

### level 3 heading
### level 3 heading
### level 3 heading [REMOVED]

* list item
* list item
* list item [REMOVED]

1. list item
2. list item
3. list item [REMOVED]

> quote
>
> quote
>
> quote [REMOVED]

[link](https://www.example.com)
[link](https://www.example.com)
[link](https://www.example.com)
[link [REMOVED]](https://www.example.com)

`inline code`
`inline code`
`inline code [REMOVED]`

```
code block
```
```
code block
```
```
code block [REMOVED]
```

```javascript
// code block with language
```
```javascript
// code block with language
```
```javascript
// code block with language
```
```javascript
// code block with language [REMOVED]
```

![image](https://fakeimg.pl/400x100/?text=image)
![image](https://fakeimg.pl/400x100/?text=image)
![image](https://fakeimg.pl/400x100/?text=image)
![image](https://fakeimg.pl/400x100/?text=image%20[REMOVED])

paragraph → heading

#### heading → paragraph

* list item → paragraph

paragraph → list item

> quote → paragraph

paragraph → quote

> quote → heading

#### heading → quote

> multi-line quote → paragraph
>
> multi-line quote → paragraph

> multi-line quote → heading
>
> multi-line quote → paragraph
Then edit it and replace its content with:
paragraph

paragraph [CHANGED]



# level 1 heading
# level 1 heading [CHANGED]


## level 2 heading
## level 2 heading [CHANGED]


### level 3 heading
### level 3 heading [CHANGED]


* list item
* list item [CHANGED]


1. list item
2. list item [CHANGED]


> quote
>
> quote [CHANGED]



[link](https://www.example.com)
[link [CHANGED]](https://www.example.com)
[link](https://www.example.com/CHANGED)


`inline code`
`inline code [CHANGED]`


```
code block
```
```
code block [CHANGED]
```




```javascript
// code block with language
```
```javascript
// code block with language [CHANGED]
```
```typescript
// code block with language
```




![image](https://fakeimg.pl/400x100/?text=image)
![image [CHANGED]](https://fakeimg.pl/400x100/?text=image)
![image](https://fakeimg.pl/400x100/?text=image%20[CHANGED])


#### paragraph → heading

heading → paragraph

list item → paragraph

* paragraph → list item

quote → paragraph

> paragraph → quote

#### quote → heading

> heading → quote

multi-line quote → paragraph
multi-line quote → paragraph


## multi-line quote → heading
multi-line quote → paragraph

This PR:

  • removes the decorations on added/changed inline code
  • fixes wrong text color applied to changed blockquotes
  • fixes the GitHub bug where no indentation is applied to the contents of changed quote blocks
  • fixes the GitHub bug where a multi-line quote block changed into a paragraph may contain some duplicated content (see Fix duplicated content and indentation in hide-markdown-diff #4052 (comment) for a minimal reproducible example)

@yakov116 yakov116 added the bug label Mar 3, 2021
@fregante
Copy link
Member

fregante commented Mar 3, 2021

I'm having a hard time seeing the problem with your demo markdown. This is what I see without this PR

Screen Shot 3

The only issue is the green text, but I see no duplicate content, I think

@cheap-glitch
Copy link
Member Author

cheap-glitch commented Mar 3, 2021

It's around the end:

With diff Without diff

image

image

The first part of the quote block gets duplicated because it's not marked as deleted.


Here's a minimal reproducible example:

Base Modified
> one [DUPLICATED]
>
> two
>
> three
one [DUPLICATED]

two
three

This will give the following diff:

With diff Without diff (broken)

image

image

Comment on lines +12 to +14
for (const changedBlockquote of select.all('.show-preview .changed > .changed_tag[data-before-tag="blockquote"]')) {
changedBlockquote.parentElement!.classList.add('ml-3');
}
Copy link
Member Author

Choose a reason for hiding this comment

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

In the end I had to rely on some JS to fix the missing indentation issue.

At first I applied a margin on any siblings of .changed_tag[data-before-tag="blockquote"] but this causes issues with inline <ins> elements, so it has to be applied to the diff block wrapper.

@cheap-glitch cheap-glitch marked this pull request as ready for review March 4, 2021 18:59
@fregante
Copy link
Member

fregante commented Mar 5, 2021

Needs a little more work, I think.

Right side: this PR

  • the duplicate line is still duplicate in this view, it just changed style

Screen Shot

  • also the code block changed style in the final result

Screen Shot 1

  • The PR title needs to be more specific than "Fix X" unless the feature is completely broken
  • The MutationObserver line needs a comment explaining why it's there or what it's waiting for

@cheap-glitch cheap-glitch marked this pull request as draft March 5, 2021 09:24
@cheap-glitch cheap-glitch changed the title Fix hide-markdown-diff Fix duplicated content and missing indentation in hide-markdown-diff Mar 5, 2021
@cheap-glitch
Copy link
Member Author

the duplicate line is still duplicate in this view, it just changed style

Actually since I switched to the CSS-only solution the style doesn't change anymore. Anyway, I thought it made more sense to hide the duplicated content only when hiding the whole diff — otherwise we're modifying the native GH behavior even when the feature isn't toggled on, which isn't really what's advertised.

@cheap-glitch cheap-glitch marked this pull request as ready for review March 6, 2021 19:23
@fregante
Copy link
Member

fregante commented Mar 7, 2021

  • the duplicate line is still duplicate in this view, it just changed style
Screen Shot

By this I meant that on the right, the line has a red background. Either we drop it or leave it unchanged since there's already a red-text line below it

@cheap-glitch
Copy link
Member Author

Since d069190 (#4052) the duplicated content isn't marked in the diff anymore. The only change is the missing indentation that's being re-added:

Vanilla RGH

image


image

Copy link
Member

@fregante fregante left a comment

Choose a reason for hiding this comment

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

It seems to work correctly

@yakov116 yakov116 changed the title Fix duplicated content and missing indentation in hide-markdown-diff Fix duplicated content and indentation in hide-markdown-diff Mar 10, 2021
@yakov116 yakov116 merged commit d8e4d26 into refined-github:main Mar 10, 2021
@cheap-glitch cheap-glitch deleted the fix-hide-markdown-diff branch March 10, 2021 15:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

Hide-markdown-diff not hiding all changed
3 participants