jw3126 / UnitfulRecipes.jl

Plots.jl recipes for Unitful.jl arrays

MIT License

37 stars 10 forks source link

added fillrange #67

Closed DanDeepPhase closed 3 years ago

DanDeepPhase commented 3 years ago

This supports fillrange in the same style as ribbon. fillrange is the lower half of the ribbon.

There is one syntax which isn't covered: plot(x, y, ribbon=[(1,5)]) the vector of tuples in this ribbon syntax creates a low bound fillrange and adds the upper bound to the data. but, it isn't caught by the fillrange or ribbon ustripattribute! call.

codecov-commenter commented 3 years ago

Codecov Report

Merging #67 (709e3f8) into master (759bf85) will increase coverage by 0.37%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master      #67      +/-   ##
==========================================
+ Coverage   86.11%   86.48%   +0.37%     
==========================================
  Files           1        1              
  Lines         108      111       +3     
==========================================
+ Hits           93       96       +3     
  Misses         15       15

Impacted Files	Coverage Δ
src/UnitfulRecipes.jl	`86.48% <100.00%> (+0.37%)`	:arrow_up:

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 465510a...709e3f8. Read the comment docs.

briochemc commented 3 years ago

Thanks for the PR! 👍

I just made a suggestion but otherwise I think this is a nice addition — thanks for catching it!

Let's wait a little to see what the others have to say but I think except from bumping the minor version number, this is good to go? 🤷

gustaphe commented 3 years ago

LGTM (with the edit). Nice feature!

briochemc commented 3 years ago

@DanDeepPhase would you mind doing the following:

if it works, test and accept the edit suggested
add a ribbon example in the docs
bump the minor version number

DanDeepPhase commented 3 years ago

I updated. I think I followed the doc style, but I don't know how to test it. This is my first pull request.

briochemc commented 3 years ago

LGTM!

but I don't know how to test it

Yep unfortunately the docs don't get deployed with each PR (unless the PR is from a collaborator with commit rights). I guess I meant to check that the plots you added in the docs work for you locally! 😃 Anyway I think let's merge this PR and we can fix any docs issue after if need be.

This is my first pull request.

Great job!

briochemc commented 3 years ago

The deployed docs (in the dev branch) look good to me so I'll trigger a new release! Thank you @DanDeepPhase!