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

Fixed bug when colour is applied in the dataset from Data View window #7355

Merged

Conversation

N-thony
Copy link
Collaborator

@N-thony N-thony commented Apr 5, 2022

Fixes #7232
@africanmathsinitiative/developers this is ready for review.
@rdstern I have fixed the bug on not setting the colour to the dataset in the Data View, in addition while testing I fixed one small bug I found in the Merge Additional Dialogue. I also realised in version 0.7.2 that with dataset you shared in Skype, the merge dialogue seems to not apply the colour though the second or first dataframe has colour applied but with this version, it seems to keep the colour if applied from one of the dataset in the merged dataset. Please could test on your side and see if I'm right? Thanks.

instat/ucrDataView.vb Outdated Show resolved Hide resolved
@N-thony N-thony changed the title Fixed bug when colour is applied Fixed bug when colour is applied in the dataset from Data View window Apr 5, 2022
@rdstern
Copy link
Collaborator

rdstern commented Apr 8, 2022

@N-thony this now seems to work, so that's a relief.
You may like to pass this on, or continue, for some of the other aspects on this dialogue.
a) The remove colour seems to work fine. But it could enable Ok, even when the receiver is empty. (You don't need to know what property made the colours to clear them.)
b) I thought you might like to remove the property from the metadata, when it is always -1?
c) When there are no colours, perhaps the default could be the Class?
d) I would still like cho9ice on the palettes to be used. This could be a useful very simple dialogue to illustrate the palettes that are usually available. (If needed, this could become another issue, to be done later.)

@N-thony
Copy link
Collaborator Author

N-thony commented Apr 11, 2022

@N-thony this now seems to work, so that's a relief. You may like to pass this on, or continue, for some of the other aspects on this dialogue. a) The remove colour seems to work fine. But it could enable Ok, even when the receiver is empty. (You don't need to know what property made the colours to clear them.) b) I thought you might like to remove the property from the metadata, when it is always -1? c) When there are no colours, perhaps the default could be the Class? d) I would still like cho9ice on the palettes to be used. This could be a useful very simple dialogue to illustrate the palettes that are usually available. (If needed, this could become another issue, to be done later.)

@rdstern I resolved items a) to c), please could you check? Thanks.

Copy link
Contributor

@ChrisMarsh82 ChrisMarsh82 left a comment

Choose a reason for hiding this comment

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

add _grid.UpdateWorksheetStyle(fillWorkSheet) back to ucrDataView

instat/ucrDataView.vb Outdated Show resolved Hide resolved
@ChrisMarsh82
Copy link
Contributor

@N-thony see the latest commit. it looks like there is an issue with reogrid when using entire range. This looks to solve it by changing the style of the top row separately. Please could you check to see if this solution fixes your issue while still allowing users to select a font through the options menu

@N-thony
Copy link
Collaborator Author

N-thony commented Apr 12, 2022

@N-thony see the latest commit. it looks like there is an issue with reogrid when using entire range. This looks to solve it by changing the style of the top row separately. Please could you check to see if this solution fixes your issue while still allowing users to select a font through the options menu

@ChrisMarsh82 thank you, I have checked by setting the colour with Colour by Property dialogue and also with the font through the options menu and it seems to work fine.
@rdstern could you re-test?

@rdstern
Copy link
Collaborator

rdstern commented Apr 17, 2022

@N-thony this seems to work well now - at least initially and visually.
a) The Remove Colour checkbox is available all the time. Should it be deleted if no colour is applied.

b) I removed colour and the result in the metadata seems odd. Visually it is ok. But it has NA for the first entry of metadata and still some numbers. That's perhaps because of the Select I applied.
image

c) So I deleted the selection and it gave an error.

image

@N-thony
Copy link
Collaborator Author

N-thony commented Apr 17, 2022

@rdstern this seems to be fixed now. Please confirm?

@rdstern
Copy link
Collaborator

rdstern commented Apr 17, 2022

@N-thony this time I tried it the other way round. So I did a select, before doing the colour. Got this:
image

Makes me wonder about doing a filter - I'll try. That seems ok.

@N-thony
Copy link
Collaborator Author

N-thony commented Apr 18, 2022

@N-thony this time I tried it the other way round. So I did a select, before doing the colour. Got this: image

Makes me wonder about doing a filter - I'll try. That seems ok.

@rdstern could you test now?

@N-thony
Copy link
Collaborator Author

N-thony commented Apr 21, 2022

@rdstern could you try this now?

@lloyddewit
Copy link
Contributor

@rdstern could you try this now?

@N-thony Before @rdstern tests again, please could you resolve my comment above (else @rdstern will need to test twice)? thanks

@Patowhiz
Copy link
Contributor

I'm now also getting this bug.
When I use 2 data frames with the similar column names, and I try to produce new columns from the dialogs that seem to also be the same across the 2 data frames. this error is thrown.

@lloyddewit
Copy link
Contributor

@rdstern could you try this now?

@N-thony Before @rdstern tests again, please could you resolve my comment above (else @rdstern will need to test twice)? thanks

@N-thony I saw that you resolved my comment, thanks

@lloyddewit
Copy link
Contributor

@N-thony Is this PR now ready for testing or do you still need to take action to address @Patowhiz 's comment above?
Thanks for clarifying

@N-thony
Copy link
Collaborator Author

N-thony commented Apr 25, 2022

@N-thony Is this PR now ready for testing or do you still need to take action to address @Patowhiz 's comment above? Thanks for clarifying

@lloyddewit I have discussed with @Patowhiz and found his comment has been fixed.

@lloyddewit
Copy link
Contributor

@rdstern Please can you test?
@ChrisMarsh82 You have an open change request above, which I think will block approval. Please can you approve (I will review/approve separately)?
Thanks

@rdstern
Copy link
Collaborator

rdstern commented Apr 26, 2022

@N-thony I got build errors?

@N-thony
Copy link
Collaborator Author

N-thony commented Apr 26, 2022

@rdstern I'm not getting the build error, let's have a quick call?

@lloyddewit
Copy link
Contributor

@rdstern I'm not getting the build error, let's have a quick call?

@N-thony I also get build errors, clsDataFramePage.vb is incorrect:
image

@N-thony
Copy link
Collaborator Author

N-thony commented Apr 26, 2022

@rdstern I'm not getting the build error, let's have a quick call?

@N-thony I also get build errors, clsDataFramePage.vb is incorrect: image

@lloyddewit thanks, I think this is after resolving the conflicts, let me fix it.

@N-thony
Copy link
Collaborator Author

N-thony commented Apr 26, 2022

@rdstern I'm not getting the build error, let's have a quick call?

@N-thony I also get build errors, clsDataFramePage.vb is incorrect: image

@lloyddewit thanks, I think this is after resolving the conflicts, let me fix it.

@lloyddewit could you try it now?

@lloyddewit
Copy link
Contributor

@N-thony Thank you for the changes. The PR now builds without errors.
@rdstern You should be able to test now

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.

Good

@lloyddewit lloyddewit added the bug label May 3, 2022
@lloyddewit lloyddewit merged commit 51a9016 into IDEMSInternational:master May 3, 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.

Complete the Experiments > Define > On-Station dialogue
5 participants