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

Climatic Summary #3818

Merged
merged 2 commits into from
Aug 18, 2017
Merged

Conversation

shadrackkibet
Copy link
Collaborator

@africanmathsinitiative/developers this is ready for review.

Copy link
Contributor

@lilyclements lilyclements left a comment

Choose a reason for hiding this comment

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

@shadrackkibet looks good.
I just noticed something small, can you run a sub which sets the station receiver as the focused receiver when the Annual rdo is checked? Otherwise, the focus may be hidden on a receiver only visible on the Within-Year rdo.

@shadrackkibet
Copy link
Collaborator Author

Should i also change the receiver focus when on "Within-Year rdo" to be the "Within-Year" Receiver?

@dannyparsons
Copy link
Contributor

I can't quite remember the dialog layout but generally we only need to change the current receiver if we are hiding the current receiver. If we're just changing other things about the dialog, no need to change this.
I'm not sure which case applies to this dialog so you'll have to decide that.

@lilyclements
Copy link
Contributor

lilyclements commented Aug 17, 2017

So we have the former: when we are on the Within-Year rdo, there is a new receiver which pops up. Hence this disappears when you click on the Annual rdo. Therefore, when you click onto the Annual rdo, we need to change the current receiver, otherwise the focus is on a non-visible receiver.

Copy link
Contributor

@lilyclements lilyclements left a comment

Choose a reason for hiding this comment

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

@dannyparsons this is ready to review

@shadrackkibet
Copy link
Collaborator Author

@dannyparsons @lilyclements how can restrict the withinYear receiver to only pick the within-Year variables i.e Month,dekad, etc .Is this possible by setting the receiver's metadata property?. I noticed there this has a bug when you allow the user to select any Element for example "Rain" into this receiver.

@lilyclements
Copy link
Contributor

I think it's okay as it is. If the user wishes to do it by "Rain", then they can - not that this is in any way advised! However, this way we can allow things like "Day", "doy_366", "Dekad" "Week", etc - variables which are generally automatically numeric.

@dannyparsons
Copy link
Contributor

That's a great point. We should restrict to those "within year types", but since we don't have record this at the moment we can't quite do it. So I think better to allow anything and explain in future we will restrict.
One question - is within year restrict to numeric now? What if my month is a factor/character? Should this be allowed? If so then I suggest no restriction on this receiver at the moment.

@lilyclements
Copy link
Contributor

There is no restriction on it at all at the moment, which is what we want for now because then the user can pick factors or numerics
I'll write an issue up on creating a within-year type set. In which case, this should be good for @dannyparsons

Private Sub ReceiverFocus()
If rdoAnnual.Checked Then
ucrReceiverStation.SetMeAsReceiver()
End If
Copy link
Contributor

Choose a reason for hiding this comment

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

This could be modified so that the current receiver only changes if the receiver that's about to be hidden is the current one. i.e. if another receiver is the current then no need to do this when changing radio buttons.
Can you look into this? but I will still merge this version now because it's a small improvement.

@dannyparsons dannyparsons merged commit 9c4b897 into IDEMSInternational:master Aug 18, 2017
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

Successfully merging this pull request may close these issues.

3 participants