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

bug in example #888

Closed
olivroy opened this issue Jun 11, 2024 · 4 comments
Closed

bug in example #888

olivroy opened this issue Jun 11, 2024 · 4 comments

Comments

@olivroy
Copy link
Contributor

olivroy commented Jun 11, 2024

tm_shape(World) + 
    	tm_polygons("HPI", 
         	fill.scale = tm_scale_continuous(), 
      		fill.chart = tm_chart_histogram(
                 position = tm_pos_out("center", "bottom"), width = 30)
          )
Error in l$position = complete_options(crt$position, o$component.position[[crt$position$type]]) : 
  object 'l' not found

Error is likely in R/step2_helper_data.R:21 or R/step2_helper_data.R:49

@olivroy
Copy link
Contributor Author

olivroy commented Jun 12, 2024

@mtennekes Thanks for fixing !

Another example is unfortunately erroring

 tm_shape(rivers) +
   	tm_lines(lwd = "strokelwd",
   			 lwd.scale = tm_scale_asis(values.scale = 0.2, value.neutral = 2),
   			 col = "scalerank",
   			 col.scale = tm_scale_categorical(values = "seaborn.dark"))
#>   Error in !legs_aes$legend[[k]]$active : invalid argument type

Error is likely in tmap/R/step4_helper_legends.R, line 100.

Thanks again, it know it's probably hard to manage such a huge task.

It is just that I'd rather have green checks before starting to try upgrading to tmap v4 in my own (extensive) tmap code.

When I have time to spend on tmap, I will propose to do messaging with cli.

  • Ability to show a message only once by session (I think the v3 -> v4 messages are extremely useful, but sometimes, you will just want to run your working v3 code, and not upgrade now. There is the ability to show each message once per session)
  • Add clickable link to help topics in error messages
  • Add color, bullets to increase message readability

Unrelated:

One thing that's really important for me (unrelated to the example issue) is the hovering feature in view mode.

In v3, the first variable would automatically be picked for hovering and assigned to id. (Robin found this not very useful and I agree).

The solution I propose for v4 is simply to show hovering when id is provided explicitly. It should not break existing code, but would remove the hovering for users who didn't opt in explicitly, so would be a good compromise.

(It should still be the title of the popup)

I will follow the main issue on this, but wanted to raise it again.

@olivroy olivroy reopened this Jun 12, 2024
mtennekes added a commit that referenced this issue Jun 13, 2024
@mtennekes
Copy link
Member

mtennekes commented Jun 13, 2024

Bug fixed (thanks for reporting)

To be honest, I don't really have a good (manual) testing method. The recent bugs are caused by a new feature I'm working on (glyphs) while being too lazy to start a new branch. Is there a simple way to test everything before pushing to the main branch? I only know devtools::check()... Or do you have other tips?

Messaging: those upgrades you propose to work on would be awesome! Thanks!

About hovering. So if I understand correctly in my post #851 (comment)

Defaults:

  • id = ""
  • popup = tm_popup(title = id, vars = TRUE) where TRUE means all variables.
    In v3 this was different: vars consisted of all data variables that were visualised, and all variables if no data was visualised. What do you prefer?
  • hover = FALSE, also when id != ""?

the last rule will be changed to:

hover = FALSE, unless id != "". Makes sense.

@olivroy
Copy link
Contributor Author

olivroy commented Jun 13, 2024

To be honest, I don't really have a good (manual) testing method. The recent bugs are caused by a new feature I'm working on (glyphs) while being too lazy to start a new branch. Is there a simple way to test everything before pushing to the main branch? I only know devtools::check()... Or do you have other tips?

I understand. I tend to be lazy too.
If you use RStudio, there is something I could recommend you: the build pane and Ctrl + Shift + T (that will trigger tests. devtools::test(). That works in background, so you can continue working while it is running. (devtools::check() can be used with Ctrl / Cmd + Shift +E and also runs in the background, but I tend to use it less, due to it being slow and taking a lot of time checking things I don't really care about.

Like I sketched back in September 2023, you could just add a dump of tests just to test for no error.

if you add this to file like tests/testthat/test-dump.R

# test easily that no regression occurs

test_that("No regression occurs", {
  skip_on_cran()
  expect_no_error({a tmap example})
  expect_no_error({a tmap example})

})

FInally, there is devtools::run_examples(), but I like it a bit less too, because it blocks my session, so can't work on other things at the same time.

About hovering. So if I understand correctly in my post #851 (comment)

That would make sense! thanks. I tend to see more clearly when I can interact with it, but basic support in v4 would be more than welcome for starters!

@olivroy
Copy link
Contributor Author

olivroy commented Jun 13, 2024

CI is green again, thanks for fixing! 🎉

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

No branches or pull requests

2 participants