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

Combined the three plot options buttons in Climatic Maps #7481

Conversation

N-thony
Copy link
Collaborator

@N-thony N-thony commented May 18, 2022

Fixes #7472
@africanmathsinitiative/developers this is ready for review.
@rdstern @lilyclements could you test this?

@@ -235,7 +235,7 @@ Public Class dlgClimaticStationMaps
AutoFillGeometry()
End Sub

Private Sub cmdPlotOptions_Click(sender As Object, e As EventArgs) Handles cmdPlotOptions.Click
Private Sub cmdPlotOptions_Click(sender As Object, e As EventArgs)
Copy link
Contributor

Choose a reason for hiding this comment

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

Out of interest, now that these do not have the "Handles ...", what does this Sub do?

Copy link
Contributor

Choose a reason for hiding this comment

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

And the other cmd*Options_Click subs

@@ -235,7 +235,7 @@ Public Class dlgClimaticStationMaps
AutoFillGeometry()
End Sub

Private Sub cmdPlotOptions_Click(sender As Object, e As EventArgs) Handles cmdPlotOptions.Click
Private Sub cmdPlotOptions_Click(sender As Object, e As EventArgs)
Copy link
Contributor

Choose a reason for hiding this comment

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

If this function does not handle any event(s), then it will never be called.
Delete this function?

instat/dlgClimaticStationMaps.vb Outdated Show resolved Hide resolved
instat/dlgClimaticStationMaps.vb Outdated Show resolved Hide resolved
@lloyddewit
Copy link
Contributor

@lilyclements I think we reviewed in parallel and came up with the same comment!
@N-thony Sorry for any confusion

@N-thony
Copy link
Collaborator Author

N-thony commented May 18, 2022

@lilyclements I think we reviewed in parallel and came up with the same comment! @N-thony Sorry for any confusion

@lloyddewit @lilyclements I resolved your comments? :)

@lloyddewit
Copy link
Contributor

@lilyclements I think we reviewed in parallel and came up with the same comment! @N-thony Sorry for any confusion

@lloyddewit @lilyclements I resolved your comments? :)

@N-thony please can you also resolve my first comment (cmdPlotOptions_Click())? thanks

@N-thony
Copy link
Collaborator Author

N-thony commented May 18, 2022

@lilyclements I think we reviewed in parallel and came up with the same comment! @N-thony Sorry for any confusion

@lloyddewit @lilyclements I resolved your comments? :)

@N-thony please can you also resolve my first comment (cmdPlotOptions_Click())? thanks

@lloyddewit I resolved your last comment.

@lloyddewit
Copy link
Contributor

@rdstern if you can test/approve, then we can merge, thanks

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.

@N-thony very nice. Much neater than before. And good that you also fixed the other bug at the same time, so it goes to the label option now.

@N-thony
Copy link
Collaborator Author

N-thony commented May 25, 2022

@shadrackkibet can you test/merge this?

@shadrackkibet
Copy link
Collaborator

I don't think I have privileges to merge PRs again. I think the process of merging PRs might have changed?

@N-thony
Copy link
Collaborator Author

N-thony commented May 25, 2022

I don't think I have privileges to merge PRs again. I think the process of merging PRs might have changed?

Okay, I will check with @ChrisMarsh82, thanks.

@ChrisMarsh82 ChrisMarsh82 merged commit 53359b4 into IDEMSInternational:master May 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Combine the three plot options buttons in Climatic Maps to one similar with the Ok button?
6 participants