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

Hide-markdown-diff not hiding all changed #4035

Closed
privatenumber opened this issue Feb 28, 2021 · 4 comments · Fixed by #4052
Closed

Hide-markdown-diff not hiding all changed #4035

privatenumber opened this issue Feb 28, 2021 · 4 comments · Fixed by #4052
Labels
bug help wanted small Issues that new contributors can pick up

Comments

@privatenumber
Copy link

Description

The hide-markdown-diff is not hiding all changes in the final result preview, and effectively shows an inaccurate result.

My changes

Before

> _💁‍♀️ Protip: Use the minify plugin in-place of the loader to transpile your JS_
>
> The `target` option tells _esbuild_ that it can use newer JS syntax to perform better minification. If you're not using TypeScript or any syntax unsupported by Webpack, you can also leverage this as a transpilation step. It will be faster because there's less files to work on and will produce a smaller output because the polyfills will only be bundled once for the entire build instead of per file.

After

#### _💁‍♀️ Protip: Use the minify plugin in-place of the loader to transpile your JS_
The `target` option tells _esbuild_ that it can use newer JS syntax to perform better minification. If you're not using TypeScript or any syntax unsupported by Webpack, you can also leverage this as a transpilation step. It will be faster because there's less files to work on and will produce a smaller output because the polyfills will only be bundled once for the entire build instead of per file.

Screenshots

Github default preview

Screen Shot 2021-02-27 at 7 31 13 PM

With "View final result" toggled

Screen Shot 2021-02-27 at 7 31 01 PM

Problems

  • The "Protip: ..." is duplicated
  • Spacing on the first "Protip: ..." is off
  • "target" is highlighted
@github-actions github-actions bot changed the title bug: hide-markdown-diff not hiding all changed Hide-markdown-diff not hiding all changed Feb 28, 2021
@fregante
Copy link
Member

If GitHub’s diff doesn’t make this easy, we have 2 options:

  • drop the feature
  • add a notice about its possible looseness

If we can detect a situation when this happens (e.g. if there’s a yellow diff?) we can display the message only in that case.

@fregante fregante added small Issues that new contributors can pick up help wanted labels Feb 28, 2021
@cheap-glitch
Copy link
Member

cheap-glitch commented Feb 28, 2021

The indentation and highlighting are easy enough to get rid of, but I don't understand why the second "Protip: […]" line isn't wrapped in a <del> element. Could this be a bug in GitHub's diff algorithm?

@fregante
Copy link
Member

Yeah that’s what I meant, the diff isn’t reliable enough and thus we need a notice. It’s not our fault

@fregante
Copy link
Member

fregante commented Mar 1, 2021

  • "target" is highlighted

We can fix this + add a notice (that should appear on next to the buttons probably)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug help wanted small Issues that new contributors can pick up
Development

Successfully merging a pull request may close this issue.

3 participants