e61-Institute / theme61

Create graphs in the e61 Institute style
https://e61-institute.github.io/theme61/
MIT License
5 stars 0 forks source link

v0.6 #76

Closed aaronw22 closed 8 months ago

aaronw22 commented 11 months ago

Describe your changes

Closes #66, closes #77, closes #63, closes #72, closes #69, closes #62, closes #70, closes #65, closes #74, closes #75, closes #67, closes #68.

Do the following before requesting a review

aaronw22 commented 10 months ago

Just realised we'll need to rewrite almost all the vignettes as well.

JackBuckley commented 10 months ago

Add a new diverging palette for scale_..._e61. This will be helpful when making maps.

JackBuckley commented 10 months ago

Could we create a default base map for different cities from OSM data? I did this for Sydney and it could be handy to pre-build this into the package if the files aren't too large.

JackBuckley commented 10 months ago

Potentially create templates for making state based charts with zoom boxes for the capital city. I've only done this for Sydney so far and it's fiddly.

JackBuckley commented 10 months ago

Do we need a new version of mpanel for the spatial maps? Or maybe we just need to not apply the auto scaling for most of the spatial maps when using the save_e61 function.

aaronw22 commented 10 months ago

RESOLVED

@JackBuckley Something in ggplot cannot handle vectors with more than 1 class.

This one is caused by graph_data$date being class IDate and Date.

Triggered on the x-var code but would be sensible to ensure it can handle these classes for both axes. Should just be a matter of adjusting the if statements to handle inputs of length > 1.

> ggplot(graph_data, aes(x = date, y = rate)) +
+   geom_line(colour = e61_teallight)
Error in if (x_var_class == "numeric" | x_var_class == "integer") { : 
  the condition has length > 1
aaronw22 commented 10 months ago

@JackBuckley

Fixed the format problem. But...

Some post-saving functionality present in single-panel save is still missing in the mpanel version.

The (invisible) return of the functions were also not the same. I have fixed that.

There's a bit of duplicate code across the two now. Likely to be some efficiencies (and reduced future copy-pasting and error-prevention from writing more helper functions for joint functionality).

Also don't like the fact that the mpanel save lives in mpanel_e61.R while single-panel lives in save_e61.R.

JackBuckley commented 10 months ago

Additional issues:

aaronw22 commented 9 months ago

@JackBuckley I think these changes have completely stopped format_flipped_bar() from working entirely.

Presumably because format_flipped_bar() works by adding some theme arguments, which would get ignored by save_e61. One solution would be to add (yet another) argument to save_e61 that formats the flipped graph more nicely.

Ignore the above. I have a different issue with flipped bars though. Which is that they aren't being scaled correctly. See graph. This would be much too small if it were shrunk to the size of a normal graph.

reg wage difference after job switch by pay set

Secondary comment moved to own comment.

aaronw22 commented 9 months ago

(Moved from another comment)

Is it time we start asking whether we are putting too much into save_e61? There may be potential to reduce the number of arguments by collapsing closely-related arguments into single arguments that take vectors or named lists.

JackBuckley commented 9 months ago

Can't seem to reply to these comments so quote replying to this one.

I agree that save_e61 has a lot of arguments, but almost all of them are there to provide detailed manual control for the expert user. Adding them to a vector would make the function a bit cleaner, but also a bit harder to use. I'm happy with how it currently is, but also not against you condensing a few things.

(Moved from another comment)

Is it time we start asking whether we are putting too much into save_e61? There may be potential to reduce the number of arguments by collapsing closely-related arguments into single arguments that take vectors or named lists.

JackBuckley commented 9 months ago

Issue where legends are removed when save_e61 is used with the autoscale functionality on.

JackBuckley commented 9 months ago

Fix issue where save_e61() overrides changes to the chart made in theme (e.g. theme(axis.text.x = element_text(angle = 45, hjust = 1)). At the moment these are overriden and convert back to the defaults.

JackBuckley commented 9 months ago

Fix issue where legends are deleted from multi-panel charts

aaronw22 commented 9 months ago

@JackBuckley something you did to the y breaks algo has broken all the tests in save_e61 fixed?

aaronw22 commented 9 months ago

Note to self: write some snapshot tests for square legend symbols done

aaronw22 commented 9 months ago

@JackBuckley keep_legend in theme_e61 still hasn't been documented (and I still don't know what it's for!)

aaronw22 commented 9 months ago

@JackBuckley this is ugly

ggplot(data.frame(x = 1:2, y = c(-24.16, 15.08)),
       aes(x, y)) +
  geom_point()
JackBuckley commented 8 months ago

@JackBuckley keep_legend in theme_e61 still hasn't been documented (and I still don't know what it's for!)

Oh yeah I think it is no longer needed (it was addressing an issue where save_e61 was overriding all user entered theme changes)

aaronw22 commented 8 months ago

@JackBuckley keep_legend in theme_e61 still hasn't been documented (and I still don't know what it's for!)

Oh yeah I think it is no longer needed (it was addressing an issue where save_e61 was overriding all user entered theme changes)

Ok cool I'll let you remove it/clean up