weirdpattern / gatsby-remark-embed-gist

Gatsby remark gists preprocessor

MIT License

30 stars 15 forks source link

feature: add parameter to select specific lines from gist #25

Closed botoxparty closed 4 years ago

botoxparty commented 4 years ago

Hey!

I was using this plug-in but needed the functionality to select specific lines so thought i'd share it back into the codebase.

It works by adding a lines parameter to the query string. e.g.

gist:botoxparty/04f9c2bd909ba6f4424386d42242f961#serverless.yml?highlights=6,56&lines=1-4,50-60

RE: Tests, it still passes but I think something has changed with the isValid check. What was the purpose of that function?

Do you have any time to write a test for this extended functionality?

Cheers! Adam

weirdpattern commented 4 years ago

Hi @botoxparty,

Thanks for sharing this! can you send me an example where you are using this? I want to see how it looks, does it just hide the lines?

botoxparty commented 4 years ago

Sure, i made a post here with a demo:

https://adamham.dev/posts/extending-gatsby-remark-embed-gist/

It removes the lines from the DOM completely, do you think it should maybe indicate to the user that there are lines removed? I guess that should probably be a separate PR.

weirdpattern commented 4 years ago

Ah this is great and thanks for the mention and link in your blog post!

I think adding an indicator would be a good idea, but I agree with you, this can be done as a separate PR.

In the mean time, I submitted a code review, overall great work, just one small optimization.

Let me know what you think 😄

codecov-io commented 4 years ago

Codecov Report

Merging #25 into master will decrease coverage by 11.84%. The diff coverage is 74.07%.

@@             Coverage Diff             @@
##           master      #25       +/-   ##
===========================================
- Coverage   97.77%   85.93%   -11.85%     
===========================================
  Files           2        2               
  Lines          45       64       +19     
  Branches       15       22        +7     
===========================================
+ Hits           44       55       +11     
- Misses          1        8        +7     
- Partials        0        1        +1

Impacted Files	Coverage Δ
src/index.js	`85.24% <74.07%> (-12.38%)`	:arrow_down:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 7668ac3...1d18435. Read the comment docs.

botoxparty commented 4 years ago

So i've made those optimisations in the latest commit. I should probably write tests for the new functionality.

I'll try and follow what you've done with the testing for highlights.

weirdpattern commented 4 years ago

Hey @botoxparty just checking on you. Are you planning on adding anything else (tests) to this PR?

Let me know 😄

botoxparty commented 4 years ago

hey @weirdpattern

so i've written out some tests based on the ones you had already written for the highlights. I'm not really sure if I've done it correctly I don't have a lot of experience with testing.

would you mind reviewing it?

weirdpattern commented 4 years ago

@botoxparty sorry for the delay. It all looks good. I'm merging now and publishing tonight