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

Small bug fixes on Import Dataset dialog #6242

Merged
merged 8 commits into from
Mar 15, 2021
Merged

Small bug fixes on Import Dataset dialog #6242

merged 8 commits into from
Mar 15, 2021

Conversation

N-thony
Copy link
Collaborator

@N-thony N-thony commented Feb 25, 2021

fixes #6229
@africanmathsinitiative/developers @rdstern this is ready for review. Thanks

lloyddewit
lloyddewit requested changes Mar 2, 2021
View reviewed changes
instat/dlgImportDataset.vb Outdated Show resolved Hide resolved
instat/dlgImportDataset.vb Outdated
Comment on lines 643 to 651
If clbSheets.CheckedItems.Count > 1 Then
ucrSaveFile.Hide()
ElseIf clbSheets.CheckedItems.Count = 1 Then
ucrSaveFile.Show()
ucrSaveFile.SetName(dctSelectedExcelSheets.Values.First(), bSilent:=True)
ElseIf clbSheets.CheckedItems.Count = 0 Then
ucrSaveFile.Show()
ucrSaveFile.SetName(frmMain.clsRLink.MakeValidText(strFileName), bSilent:=True)
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.

Replace with a select/case statement?

@N-thony
Copy link
Collaborator Author

N-thony commented Mar 2, 2021

@lloyddewit thanks for the comment above. I think have resolved it.

lloyddewit
lloyddewit previously approved these changes Mar 2, 2021
View reviewed changes
@lloyddewit
Copy link
Contributor

@rdstern please could you test? thanks

rdstern
rdstern previously approved these changes Mar 7, 2021
View reviewed changes
@shadrackkibet
Copy link
Collaborator

I tested and found some minor design issues that should be easy to fix here instead of another PR. See the below screenshot.

  • There is a control that overlaps with preview when loading the .dly file
  • Remove unnecessary space between the label and input in the save control

image

@rdstern
Copy link
Collaborator

rdstern commented Mar 7, 2021

@shadrackkibet sounds good

@N-thony N-thony dismissed stale reviews from rdstern and lloyddewit via 5cdfeec March 7, 2021 19:53
@N-thony
Copy link
Collaborator Author

N-thony commented Mar 7, 2021

@shadrackkibet thanks for your comments. I think have resolved them? could you test?

@shadrackkibet
Copy link
Collaborator

@N-thony can you edit this PR title so that it contains the details of the bug fixed and the changes made? Check the message below from Danny sent to your mailbox.

  1. Pull Request Titles
    The PR title will be used in the release notes. Please make sure the title of your PR is descriptive of the change you are making (but not too long). Also, try to make it less technical as possible. These will be read by users, so imagine a user without programming knowledge reading your PR title. Will they understand what this actually changes in the software? Sometimes it's unavoidable that the title will be technical, but for most dialog changes, we should be able to make them clear to users. In general, include the dialog name in the title if the PR is on a specific dialog.

Examples
Good: Corrected consecutive wet days calculation on rainfall quality control
Good: Cumulative/Exceedance Graph: Added points option
Good: Small bug fixes on NetCDF dialog
Bad: issue #8888
Bad: Bug fix
Bad: dlgMakeDate

The PR title can be edited after its created so you can improve it if you think of a more appropriate title. Reviewers should also review the PR title and check it's a sensible description.

@N-thony N-thony changed the title Fixing bug in the File > Open from File dialogue Small bug fixes on Import Dataset dialog Mar 8, 2021
@shadrackkibet
Copy link
Collaborator

Another problem is that the new data frame name is not retained on reopen. It is always reset to the default name.

@N-thony
Copy link
Collaborator Author

N-thony commented Mar 8, 2021

Another problem is that the new data frame name is not retained on reopen. It is always reset to the default name.

@shadrackkibet thanks for your comment, would you come again. I have tried this and the new data frame name is retained on reopening.

@shadrackkibet
Copy link
Collaborator

Another problem is that the new data frame name is not retained on reopen. It is always reset to the default name.

@shadrackkibet thanks for your comment, would you come again. I have tried this and the new data frame name is retained on reopening.

  • Open a file say .csv from your directory
  • Change the New Data frame name in the dialog to say "myData"
  • Click Ok / Close the dialog
  • Now reopen the dialog

Notice that the name ("myData") is lost.

@N-thony
Copy link
Collaborator Author

N-thony commented Mar 8, 2021

Another problem is that the new data frame name is not retained on reopen. It is always reset to the default name.

@shadrackkibet thanks for your comment, would you come again. I have tried this and the new data frame name is retained on reopening.

  • Open a file say .csv from your directory
  • Change the New Data frame name in the dialog to say "myData"
  • Click Ok / Close the dialog
  • Now reopen the dialog

Notice that the name ("myData") is lost.

@shadrackkibet I think it is fine since the New Data frame name always by default will be the file name. If you click on a file, it sets the name of the file.

@lloyddewit
Copy link
Contributor

@shadrackkibet I think it is fine since the New Data frame name always by default will be the file name. If you click on a file, it sets the name of the file.

@N-thony As a general principle, if the user closes a dialog, and then reopens it later, the dialog should return to the state it was in when it was closed. Therefore I think @shadrackkibet 's comment from 08 March is still open. Is it difficult to fix?

@N-thony
Copy link
Collaborator Author

N-thony commented Mar 10, 2021

@lloyddewit thanks for your comment. I think have resolved it.

@lloyddewit
Copy link
Contributor

@shadrackkibet I think it is fine since the New Data frame name always by default will be the file name. If you click on a file, it sets the name of the file.

@N-thony As a general principle, if the user closes a dialog, and then reopens it later, the dialog should return to the state it was in when it was closed. Therefore I think @shadrackkibet 's comment from 08 March is still open. Is it difficult to fix?

@N-thony I tested and this now seems to be resolved - thanks

lloyddewit
lloyddewit previously approved these changes Mar 12, 2021
View reviewed changes
@lloyddewit
Copy link
Contributor

@rdstern please could you retest? thanks

rdstern
rdstern requested changes Mar 12, 2021
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.

The problem is changed but partly still remains.
I opened an Excel file from the library as described before - it worked fine.
Then I tried to open a new file and it gave me the provisional name of the previous one. This is an occasion when the name freom before should be updated, rather than just remembered.

@N-thony
Copy link
Collaborator Author

N-thony commented Mar 13, 2021

@rdstern thanks for your comment above. I think have resolved it. Please could you test?

@lloyddewit lloyddewit merged commit df3d47b into IDEMSInternational:master Mar 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lloyddewit lloyddewit lloyddewit approved these changes

@rdstern rdstern rdstern approved these changes

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

Successfully merging this pull request may close these issues.

Another bug in the File > Open from File dialogue
4 participants
@N-thony @lloyddewit @shadrackkibet @rdstern