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

Added option to compare by row in the Compare Columns dialog #6433

Conversation

HawardKetoyoMsatsi
Copy link
Contributor

@HawardKetoyoMsatsi HawardKetoyoMsatsi commented Apr 28, 2021

@shadrackkibet @africanmathsinitiative/developers @rdstern This is ready for review.
This PR addresses issues suggested by @shadrackkibet in PR #6414 .

@lloyddewit
Copy link
Contributor

@HawardKetoyoMsatsi Thank you for the PR. When you open a PR, please could you add more details about what has changed. Is this a bug fix or enhancement? Is it related to an specific GitHub issue?

@N-thony Please could you peer review?

Thanks

@N-thony
Copy link
Collaborator

N-thony commented Apr 29, 2021

@HawardKetoyoMsatsi Thank you for the PR. When you open a PR, please could you add more details about what has changed. Is this a bug fix or enhancement? Is it related to an specific GitHub issue?

@N-thony Please could you peer review?

Thanks

@lloyddewit I'm happy to do it.

instat/dlgCompareColumns.vb Outdated Show resolved Hide resolved
instat/dlgCompareColumns.vb Outdated Show resolved Hide resolved
instat/dlgCompareColumns.vb Outdated Show resolved Hide resolved
instat/dlgCompareColumns.vb Outdated Show resolved Hide resolved
rdstern
rdstern previously approved these changes Apr 30, 2021
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.

Looks ok to merge to me!

@HawardKetoyoMsatsi
Copy link
Contributor Author

@N-thony I have fixed the issue raised concerning unnecessary comments.
@lloyddewit this PR only addresses issues suggested by @shadrackkibet in PR #6414

@Patowhiz
Copy link
Contributor

Patowhiz commented May 5, 2021

@HawardKetoyoMsatsi in this PR does this dialog expand and shrink it's size based on the radio button checked? If do, then the only thing left is linking the receiver to the ucrSave control as outlined by @rdstern in issue #6397. For this dialog, I think this should just be 1 line as done in most dialogs in PR #6434, we can discuss about it further.

@Patowhiz
Copy link
Contributor

Patowhiz commented May 5, 2021

@HawardKetoyoMsatsi in regards to my comment above, ignore it. After looking at your code the receiver is already linked and working correctly.

Copy link
Contributor

@lloyddewit lloyddewit left a comment

Choose a reason for hiding this comment

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

Thanks, this looks good, just a few small suggestions

ucrBase.clsRsyntax.SetBaseRFunction(clsIfElseCompareFunction)
Me.Size = New System.Drawing.Size(Me.Width, iDialogHeight)
ucrBase.Location = New Point(ucrBase.Location.X, iBaseMaxY)
ucrSaveLogical.Location = New Point(ucrSaveLogical.Location.X, iBaseMaxY / 1.07)
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we just set the 'Y' location to its original value?

Suggested change
ucrSaveLogical.Location = New Point(ucrSaveLogical.Location.X, iBaseMaxY / 1.07)
ucrSaveLogical.Location = New Point(ucrSaveLogical.Location.X, iBaseMaxY)

Copy link
Contributor

Choose a reason for hiding this comment

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

@HawardKetoyoMsatsi You marked this as resolved but the code was not changed. Please could you clarify?

instat/dlgCompareColumns.vb Show resolved Hide resolved
instat/dlgCompareColumns.vb Outdated Show resolved Hide resolved
instat/dlgCompareColumns.vb Show resolved Hide resolved
instat/dlgCompareColumns.vb Outdated Show resolved Hide resolved
instat/dlgCompareColumns.vb Show resolved Hide resolved
rdstern
rdstern previously approved these changes May 17, 2021
Copy link
Contributor

@lloyddewit lloyddewit left a comment

Choose a reason for hiding this comment

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

Thanks! There was just one very small comment so I approved

@lloyddewit
Copy link
Contributor

@rdstern please could you retest/approve? thanks

@lloyddewit
Copy link
Contributor

@shadrackkibet I'm happy to merge, but if you'd like to review first, then please go ahead

@shadrackkibet
Copy link
Collaborator

@lloyddewit looks good.

@shadrackkibet shadrackkibet merged commit a50a30f into IDEMSInternational:master May 24, 2021
@dannyparsons dannyparsons changed the title Updated compare columns dialog Added option to compare by row in the Compare Columns dialog May 26, 2021
@dannyparsons
Copy link
Contributor

@HawardKetoyoMsatsi A more descriptive title for the PR would be useful in future as these appear in the release notes. "Updated" is a bit too vague.

@HawardKetoyoMsatsi HawardKetoyoMsatsi deleted the updated_compare_columns_dialog branch May 28, 2021 08:39
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.

7 participants