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

feat: Support hide lines in remark-embed-snippet #6084

Merged
merged 41 commits into from
Dec 31, 2018

Conversation

washingtonsoares
Copy link
Contributor

@washingtonsoares washingtonsoares commented Jun 22, 2018

For a more detailed explanation of this improvement, refer to the README.

This PR adds the ability to define rows to be hidden.

Given a file like this (eg examples/hello-world.js):

// hideline-range{1-3,9-11}
import React from "react"
import ReactDOM from "react-dom"

function App() {
  return (
    <h1>Hello world!</h1>
  )
}

const rootElement = document.getElementById("root")
ReactDOM.render(<App />, rootElement)

Will generate a code like this:
screenshot from 2018-06-21 20-21-58

This PR is related to PR #5619 and ReactJS.org PR reactjs/react.dev/pull/913

cc @cyan33

@washingtonsoares washingtonsoares changed the title Support hide lines Support hide lines in remark-embed-snippet Jun 22, 2018
it(`should support hideline-range markers`, () => {
fs.readFileSync.mockReturnValue(
`
// hideline-range{1-2, 9-11, 16}
Copy link
Contributor

@cyan33 cyan33 Jun 22, 2018

Choose a reason for hiding this comment

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

The white space after the comma should be removed to keep consistency.

@cyan33
Copy link
Contributor

cyan33 commented Jun 22, 2018

Can you add a test case where hideline and highlight both exist?

@gatsbybot
Copy link
Collaborator

gatsbybot commented Jun 22, 2018

Deploy preview for gatsbygram ready!

Built with commit 7324d39

https://deploy-preview-6084--gatsbygram.netlify.com

@gatsbybot
Copy link
Collaborator

gatsbybot commented Jun 22, 2018

Deploy preview for using-drupal ready!

Built with commit 7324d39

https://deploy-preview-6084--using-drupal.netlify.com

@cyan33
Copy link
Contributor

cyan33 commented Jun 22, 2018

I'm actually not quite sure about the expected behavior in the first place when we have both hideline and highlight-line at the same time. How should the number of hideline affect those in highlight-line?

Let's check with @bvaughn

@washingtonsoares
Copy link
Contributor Author

washingtonsoares commented Jun 23, 2018

@cyan33 I do not know if I understand correctly, but I believe the number of hidden lines should not influence the highlight line setting.

For example:

// hideline-range{1-3}
// highlight-range{4-8} - 
import React from "react"
import ReactDOM from "react-dom"

function App() {
  return (
    <h1>Hello world!</h1>
  )
}

Lines 1 to 3 will be hidden, so we could think of writing //highlight-range{1-5}. But I think it's more consistent to leave //highlight-range{4-8}.

@cyan33
Copy link
Contributor

cyan33 commented Jun 23, 2018

@washingtonsoares Agree. I've read the changes. Looks good to me. 👍

```

Will produce something like this:
![screenshot from 2018-06-21 20-21-58](https://user-images.githubusercontent.com/5726140/41750161-cdf430cc-7590-11e8-9ccb-8829bcce0f14.png)
Copy link
Contributor

Choose a reason for hiding this comment

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

won't this url expire after some time?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @pieh! 🙂
I did not know that.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was also not aware of that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, this was just question, not a statement - sorry for confusion here!

@bvaughn
Copy link
Contributor

bvaughn commented Jun 25, 2018

I think the expected behavior when highlight and hide lines overlap is ambiguous, and we should error if the ranges ever intersect.

@washingtonsoares
Copy link
Contributor Author

@bvaughn I got it and come to think of it, it seems to make sense.

However in this case it is better to throw the exception or just console.error ?

@bvaughn
Copy link
Contributor

bvaughn commented Jun 26, 2018

I think it's better to error.

@washingtonsoares
Copy link
Contributor Author

washingtonsoares commented Jun 27, 2018

@bvaughn @cyan33 I have done the requested changes. 🙂

const hasIntersection = isHiddenLine && isHighlightedLine

if (hasIntersection) {
throw `There can be no intersection between hidden lines and highlighted lines`
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we display path to file that is offender here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It sounds like a good idea.

const hasIntersection = isHiddenLine && isHighlightedLine

if (hasIntersection) {
throw Error(`There can be no intersection between hidden lines and highlighted lines - "${path}"`)
Copy link
Contributor

@cyan33 cyan33 Jun 27, 2018

Choose a reason for hiding this comment

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

What about The range number of hide-lines and highlight-lines cannot be intersected. Check the file: ${path}.

@pieh
Copy link
Contributor

pieh commented Jun 28, 2018

I have question about DX - does this terminate both gatsby build and gatsby develop when "The range number of hide-lines and highlight-lines cannot be intersected" error is thrown? In general we want to terminate gatsby build on errors, but keep gatsby develop running so users can fix error without restarting it (but I guess this is tricky here, because I don't think hot reloading works for snippets content?)

@washingtonsoares
Copy link
Contributor Author

@bvaughn @cyan33 What do you think ?

@pieh
Copy link
Contributor

pieh commented Jul 6, 2018

I feel like this is good to be merged now - is there anything else to do here?

@cyan33
Copy link
Contributor

cyan33 commented Jul 6, 2018

LGTM as well.

packages/gatsby-remark-embed-snippet/README.md Outdated Show resolved Hide resolved
packages/gatsby-remark-embed-snippet/README.md Outdated Show resolved Hide resolved
packages/gatsby-remark-embed-snippet/src/index.js Outdated Show resolved Hide resolved
packages/gatsby-remark-embed-snippet/src/index.js Outdated Show resolved Hide resolved
@KyleAMathews
Copy link
Contributor

Deploy preview for using-postcss-sass failed.

Built with commit 7324d39

https://app.netlify.com/sites/using-postcss-sass/deploys/5b57b5b03813f0336b68bb8a

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.

8 participants