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

Ensured logical layout and correct toggling of Script, Metadata, Data and Output windows #8504

Merged
merged 6 commits into from
Sep 14, 2023

Conversation

Patowhiz
Copy link
Contributor

@Patowhiz Patowhiz commented Aug 22, 2023

When you start R-Instat, there is a layout oddity as shown in the image below.
This PR fixes the oddity and also comment #8465 (review)
@africanmathsinitiative/developers this is ready for review.

Layout oddity screenshot
1

@Patowhiz
Copy link
Contributor Author

This can only merged after PR #8465 has been merged. It contains changes from that PR.

@rdstern I have now fixed the layout oddity you saw in PR #8465 when you now open the column metadata.

rdstern
rdstern previously approved these changes Aug 22, 2023
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.

FRom what I could see, this looks fine. @lloyddewit please note Patrick's message that the other pull request needs to be merged before this one.

@lloyddewit
Copy link
Contributor

@Patowhiz Thank you for this PR. I started to review but it is difficult to see which changes relate to this PR and which ones relate to PR #8465.
I suggest that we merge PR #8465 first before I review.
Thanks

@Patowhiz
Copy link
Contributor Author

@lloyddewit agreed.

rdstern
rdstern previously approved these changes Aug 30, 2023
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.

@Patowhiz this looks good. I note it also incorporates the extra information on the number of variables in a multiple receiver.
However, this number is not visible if the sheet name is long. I wonder if you do an abbreviation of the sheet name. I don't suppose the R abbreviate function can be used here, but just giving say the first 8 characters, or perhaps 6 followed by dot, dot, dot, if there is an abbreviation, might be enough. The data-frame is anyway on the left in the data selector, so it isn't vital that the full name is repeated.

Happy to approave, but not sure of the oder that the rewlated pull requests should be merged. I think in ascending number.

@Patowhiz
Copy link
Contributor Author

@rdstern yes we can look into how to solve long data frame names in a separate PR. Thanks.

updated branch with changes from the master
updated branch with changes from master
@Patowhiz
Copy link
Contributor Author

@rdstern @lloyddewit this is now ready for review.

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.

@Patowhiz this looks much cleaner and more professional now. Perhaps I imagine, but it also seems to load faster now?
One "curiosity" has been that sometimes the first time I load the library it closes the R-Instat window. When I click on the icon in the taskbar it all returns and the library control is ready to continue.
I don't know if your changes affected that? It happened the first time I started, but not afterwards?
I am approving. @lloyddewit you were waiting until the wide data changes were merged. I think that has now been done?

@lloyddewit lloyddewit changed the title Removes main form layout oddity on startup and when toggling visibility of it's controls (Script, Metadata, Data and Output windows) Ensured logical layout and correct toggling of Script, Metadata, Data and Output windows Sep 14, 2023
@lloyddewit lloyddewit added the bug label Sep 14, 2023
@lloyddewit lloyddewit merged commit ee889f9 into IDEMSInternational:master Sep 14, 2023
2 checks passed
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.

4 participants