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

Improved the Climatic > PICSA > Crops Dialog Layout #9407

Merged
merged 7 commits into from
Feb 10, 2025

Conversation

MeSophie
Copy link
Contributor

@MeSophie MeSophie commented Jan 31, 2025

Fixes #9384
@rdstern @N-thony I improved the PICSA > Crops dialog layout By adding a second selector. I will finalize the label change for the first selector on another PR because the change is on the data base. Please have a look.

@rdstern
Copy link
Collaborator

rdstern commented Jan 31, 2025

@MeSophie well done. That's looking great.
a) The controls from the daily selector still fill automatically. If there are default names for the start and end of the rains/season, could they fill too? If both end rains and end season, then use end season. Only if that is easy.
b) The dialog is quite wide, could you decrease the space between the Add buttons and the blocks a bit.
c) Could you change the pull-down controls, to never have more than 3 values, then make them smaller, and again, make the dialog less wide. I suggest as follows:

  1. For the Start omit the second option, namely 80, 90, 100, 110, 120.
  2. For the Amounts omit the second, namely 300, 400, 500, 600, 700.
  3. For the Lengths omit the second, namely 100, 110, 120, 130, 140. Also change the third into 100, 140, 10.

And a question for you, to consider in both English and French. I think it would still be clear is we changed those labels into single words - and then we could save more space. Would they still be reasonably clear - and remember we will soon have the Help information as well, if needed!

Could they be Planting, Amounts and Length?

I fully understand that changing the labels will be in a second pull request. I like this separation of the tasks,

We could also add tooltips e.g. Planting: The day number for planting. Starting from January, April 1st is day 92. Starting from July, November 1st is day 124.
Amounts: The amount of water (rainfall) needed for the crop. Usually between 250mm and 1000mm.
Length: The crop duration, in days. Often between 60 days (2 months) and 150 days (5 months).

@MeSophie
Copy link
Contributor Author

MeSophie commented Feb 3, 2025

image

@rdstern Could you please describe this part in more detail? How are we going to implement it in the code? I tried replacing planting day with ‘April 1st’ and got an error apparently it doesn't take strings?

@rdstern
Copy link
Collaborator

rdstern commented Feb 3, 2025

@MeSophie I wonder if you have added tooltips to a dialog before? If not, then have a look at the calculator, almost any keyboard, that has many tooltips. Or was that not the question?

@MeSophie
Copy link
Contributor Author

MeSophie commented Feb 4, 2025

@rdstern This PR is ready to be test. Please have a look.

rdstern
rdstern previously approved these changes Feb 5, 2025
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.

@MeSophie this looks great - well done indeed. @lilyclements may like to look too, but I am approving anyway, so that could wait until it is merged.
@N-thony this would be very good to try and include in the new version this week.

Copy link
Collaborator

@N-thony N-thony left a comment

Choose a reason for hiding this comment

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

@MeSophie just few comments

@@ -23,6 +25,11 @@ Public Class dlgPICSACrops
Public bFirstLoad As Boolean = True
Private bReset As Boolean = True
Private strCurrDataName As String = ""
Private lstEndReceivers As New List(Of ucrReceiverSingle)
Private lstStartReceivers As New List(Of ucrReceiverSingle)
Private isFilling As Boolean = False
Copy link
Collaborator

Choose a reason for hiding this comment

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

Our naming convention for boolean variables starts with a lowercase 'b', e.g., bIsFilling.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is solved.

ucrReceiverYear.SetRCode(clsCropsFunction, bReset)
ucrReceiverStation.SetRCode(clsCropsFunction, bReset)
ucrReceiverRainfall.SetRCode(clsCropsFunction, bReset)
ucrReceiverDay.SetRCode(clsCropsFunction, bReset)
'ucrReceiverStart.SetMeAsReceiver()
ucrSelectorForCrops.SetDataframe(ucrReceiverStart.GetDataName())
' ucrSelectorForCrops.SetDataframe(ucrReceiverStart.GetDataName())
Copy link
Collaborator

Choose a reason for hiding this comment

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

remove?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is solved.

@@ -218,15 +249,16 @@ Public Class dlgPICSACrops
'TODO This should be done further done.
' This ensures the correct data frame is set before attempting to fill the receiver
'ucrReceiverYear.SetMeAsReceiver()
ucrSelectorForCrops.SetDataframe(ucrReceiverYear.GetDataName())
'ucrSelectorForCrops.SetDataframe(ucrReceiverYear.GetDataName())
Copy link
Collaborator

Choose a reason for hiding this comment

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

remove?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is solved.

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.

Thanks @MeSophie I'm approving again
@N-thony back to you

@N-thony N-thony merged commit fa2fcf3 into IDEMSInternational:master Feb 10, 2025
2 checks passed
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.

Improving the Climatic > PICSA > Crops dialog layout
3 participants