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

224 correct vignette visualization deployment #229

Merged
merged 29 commits into from
Jul 24, 2023

Conversation

PietrH
Copy link
Member

@PietrH PietrH commented Jul 13, 2023

The vignette: visualize-deployment-features.Rmd has gotten out of date, zero count observations are displayed by default as black multiplication symbols ×, not red circle markers.

After discussion with Damiano we've decided to update the vignette, not the function map_dep().

The function used to display red circle markers for zero count observations, but was updated to use an icon instead. This vignette was overlooked.

image


Damiano noticed in this PR that an older issue is actually unsolved: #6 (comment) :

  • map_dep() can differentiate between NA and 0
  • map_dep() visualizes NA and 0 differently
    map_dep() offers users control over NA and 0 visualization:
  • na_values_show
  • na_values_icon_url
  • na_values_icon_size
    Assertions were added for these last arguments.

@PietrH PietrH linked an issue Jul 13, 2023 that may be closed by this pull request
PietrH and others added 2 commits July 13, 2023 15:55
@PietrH PietrH added the documentation Improvements or additions to documentation label Jul 13, 2023
@PietrH
Copy link
Member Author

PietrH commented Jul 13, 2023

@damianooldoni Could you have a look at the rest of the vignette? Especially the mentions of the colours red or grey.

For example, is this still correct? I don't see any grey circles:

image

@PietrH PietrH marked this pull request as ready for review July 18, 2023 08:04
@damianooldoni damianooldoni self-requested a review July 20, 2023 14:53
Copy link
Member

@damianooldoni damianooldoni left a comment

Choose a reason for hiding this comment

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

Thansk @PietrH to ask me to review the vignette. It was time to update it. And the difference between NAs and 0 while visualizing the number of species was gone, apparently. I think I fixed it.
I would like to have your review. But I cannot point you as reviewer officially as you are the owner of this PR 🤣

Copy link
Member Author

@PietrH PietrH left a comment

Choose a reason for hiding this comment

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

@damianooldoni, I've suggested some minor changes.

Mostly about test coverage, if you want we can split that off to a different pull request. In that last case I can contribute.

Do you think we should test the output of map_dep() for certain properties, such as what leaflet calls were used and so forth? Such a test might notify us of problems such as not plotting things we expect to be plotted, but is less strict than a snapshot.

R/map_dep.R Outdated Show resolved Hide resolved
R/map_dep.R Show resolved Hide resolved
tests/testthat/test-map_dep.R Show resolved Hide resolved
tests/testthat/test-map_dep.R Show resolved Hide resolved
tests/testthat/test-map_dep.R Outdated Show resolved Hide resolved
@damianooldoni damianooldoni merged commit c4f6a80 into main Jul 24, 2023
10 checks passed
@damianooldoni damianooldoni deleted the 224-correct-vignette-visualization-deployment branch July 24, 2023 15:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
2 participants