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

Bug Fixes for Excel range settings #9452

Merged
merged 3 commits into from
Feb 26, 2025

Conversation

derekagorhom
Copy link
Contributor

@derekagorhom derekagorhom commented Feb 19, 2025

Fixes #9370
Fixes #9293
I have implemented the range option when selecting multiple excel sheets and also when importing the default is to tick the first sheet.
@rdstern , @N-thony can you review this
Thanks

@jkmusyoka
Copy link
Contributor

@derekagorhom Were these changes ready to be tested? The range option still does not work for multiple excel sheets. And the first sheet is not ticked by default.

@derekagorhom
Copy link
Contributor Author

@jkmusyoka below are screenshots from my PR
when you load the dataset into the import dialog:
image
when you select the range:
image

@jkmusyoka
Copy link
Contributor

@derekagorhom that's interesting!

@rdstern can you test these changes from your end and let us know? Maybe I am missing something.

I am using Excel file from Petauke to test. It is attached.

For the range, specify A4:G35 and try to import any 2 sheets in the file into R-Instat.

The changes made by Derrick don't seem to appear on my end.

petauke missing data moffat.xlsx

@rdstern
Copy link
Collaborator

rdstern commented Feb 21, 2025

@derekagorhom I am adding the file I used for testing:

msekera agro met.zip

Here is what it looks for me:

image

a) Good that the first sheet is selected.
b) But not-so-good, that underneath it says No Sheet Selected - which is is! And also Preview not implemented for this file type because there is a preview option for excel file types.
c) Good: I then selected Oct2022 instead - and that looks fine as below:

image

d) Not so Good: Can you see the focus is still on the first row. I could not get to this (from Version 0.8.2):

image

e) Also not good. When I was in October 2022 and changed the Rows to skip from 0 to 1, then I lost the preview.

f) And when I ticked the 5th sheet it also (as a bonus) ticked the first sheet for me. You seem very insistent that the first sheet should be re-ticked at various opportunities!

f) So, at the beginning the first sheet should have the focus. After that it is ok if it loses the focus. And the getting the focus should noit interact with the other controls.

g) Now to the range: That's A4 to G35 for the 4th and 5th sheets. Aha, that now seems to work. Well done on that bit.

@derekagorhom
Copy link
Contributor Author

@rdstern please i have made the changes
can you review them
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.

@derekagorhom well done.
@jkmusyoka do you want to check too, or wait till it is merged?
@N-thony could you check the code, and merge if ok. Thanks

@N-thony
Copy link
Collaborator

N-thony commented Feb 26, 2025

@rdstern @derekagorhom, when I import the file you have attached above, I get this with the first sheet selected but the OK button is disable. I had to uncheck and check/select this sheet or any other to get the OK button enabled.
image

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 and well spotted @N-thony. Good that we need 2 checkers before merging and I really should have noticed that myself. I must be more careful in the future.

@derekagorhom
Copy link
Contributor Author

@rdstern , @N-thony Please i have implemented the ok button when the first sheet is checked

@N-thony N-thony merged commit 7a544b3 into IDEMSInternational:master Feb 26, 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
4 participants