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.

Sign up for GitHub

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

Jump to bottom

Added missing Geoms to the plot options in the Climatic>Mapping>Map sub dialog #7506

Merged
merged 5 commits into from Jun 5, 2022
Merged

Added missing Geoms to the plot options in the Climatic>Mapping>Map sub dialog #7506

merged 5 commits into from Jun 5, 2022

Conversation

EstherNjeriLiberatta
Copy link
Contributor

Fixes #7494
@rdstern @africanmathsinitiative/developers This is ready for review.

@rdstern
Copy link
Collaborator

rdstern commented May 27, 2022

@EstherNjeriLiberatta very well done, that looks great.
I was surprised, though, in the order of the layers. It had the labels first, then the points and third was the sf. I expected them to be the other way round.
Then I tried changing the label size to 0.3 and that gave me the following error, see below. I wonder if that is because of the ordering of the layers?
image

@N-thony
Copy link
Collaborator

N-thony commented Jun 2, 2022 •
edited

@EstherNjeriLiberatta very well done, that looks great. I was surprised, though, in the order of the layers. It had the labels first, then the points and third was the sf. I expected them to be the other way round. Then I tried changing the label size to 0.3 and that gave me the following error, see below. I wonder if that is because of the ordering of the layers? image

@EstherNjeriLiberatta thanks for this, is it ready again for review?

@EstherNjeriLiberatta
Copy link
Contributor Author

@rdstern The error was coming about when you edit into the sub dialogue which was causing the data to be lost in the geom_point or geom_label_repel code. It is now fixed by adding the dataset to the ggplot() function. The order of the geoms is also fixed. This is ready for review.
@N-thony I have fixed the issue, it's now okay. Thanks

rdstern
rdstern requested changes Jun 2, 2022
View reviewed changes
Copy link
Collaborator

@rdstern rdstern left a comment

Choose a reason for hiding this comment

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

@EstherNjeriLiberatta that's good and seems to work fine.
Ideally, the Point Options and Label Repel options would also be included in the drop-down.
If that is tricky, then we could merge this and consider the extra geoms as an additional issue?

@EstherNjeriLiberatta
Copy link
Contributor Author

@rdstern I suggest we open another issue on this and merge this one. Thank you

rdstern
rdstern previously approved these changes Jun 3, 2022
View reviewed changes
Copy link
Collaborator

@rdstern rdstern left a comment

Choose a reason for hiding this comment

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

@EstherNjeriLiberatta that's good and seems to work fine.
Ideally, the Point Options and Label Repel options would also be included in the drop-down.
If that is tricky, then we could merge this and consider the extra geoms as an additional issue?

After your comment I am approving and perhaps you could make a new issue of the point about adding the remaining geoms into the pull-down.

lloyddewit
lloyddewit requested changes Jun 3, 2022
View reviewed changes
instat/dlgClimaticStationMaps.vb
ChangeSize()

End Sub

Private Sub SetDefaults()
clsParamOperator.Clear()
Copy link
Contributor

Choose a reason for hiding this comment

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

Also delete declaration on line 48?

instat/dlgClimaticStationMaps.vb Outdated
Comment on lines 338 to 339
Else
clsGGplotOperator.RemoveParameterByName("geom_label")
Copy link
Contributor

Choose a reason for hiding this comment

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

parameter already removed above. Do we need these lines?

instat/dlgClimaticStationMaps.vb Outdated
Comment on lines 343 to 344
Else
clsGGplotOperator.RemoveParameterByName("facets")
Copy link
Contributor

Choose a reason for hiding this comment

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

parameter already removed above. Do we need these lines?

instat/dlgClimaticStationMaps.vb Outdated
Comment on lines 348 to 349
Else
clsGGplotOperator.RemoveParameterByName("scale_shape_manual")
Copy link
Contributor

Choose a reason for hiding this comment

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

parameter already removed above. Do we need these lines?

@EstherNjeriLiberatta
Copy link
Contributor Author

@lloyddewit I have resolved your comments above. Thank you.

lloyddewit
lloyddewit approved these changes Jun 5, 2022
View reviewed changes
Copy link
Contributor

@lloyddewit lloyddewit left a comment

Choose a reason for hiding this comment

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

@EstherNjeriLiberatta Thank you, looks good

@lloyddewit
Copy link
Contributor

@rdstern Some code changes since your last approval. In theory, the functionality should not be affected. If you can test/reapprove, then we can merge. Thanks

@lloyddewit lloyddewit changed the title Added missing Geoms in the plot options sub dialogue. Climatic>Mapping>Map. Added missing Geoms to the plot options in the Climatic>Mapping>Map sub dialog Jun 5, 2022
@lloyddewit lloyddewit merged commit 183436c into IDEMSInternational:master Jun 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rdstern rdstern rdstern approved these changes

@lloyddewit lloyddewit lloyddewit approved these changes

Assignees
No one assigned
Labels
feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Improve the mapping dialogue in R-Instat
4 participants
@EstherNjeriLiberatta @rdstern @N-thony @lloyddewit