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

Improvements to Stack(Pivot Longer) dialog #6876

Merged
merged 14 commits into from Nov 26, 2021

Conversation

Vitalis95
Copy link
Contributor

@Vitalis95 Vitalis95 commented Oct 21, 2021

Partially solves #6868
@rdstern @shadrackkibet, this is for the Drop Missing Values Checkbox. Have a look at it.

@lloyddewit
Copy link
Contributor

@N-thony Please could you do the first peer review? thanks

instat/dlgStack.vb Outdated Show resolved Hide resolved
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.

I really like the fact this now has new features from the pivot_longer function.
But I assume that the old feature of being able to stack multiple sets is still to be re-instated? I would be happy for it to be done "in the old way" so it doesn't necessarily use pivot_longer. I suggest we only use the tidyverse functions when they are convenient. It isn't a rule.

@shadrackkibet
Copy link
Collaborator

I really like the fact this now has new features from the pivot_longer function. But I assume that the old feature of being able to stack multiple sets is still to be re-instated? I would be happy for it to be done "in the old way" so it doesn't necessarily use pivot_longer. I suggest we only use the tidyverse functions when they are convenient. It isn't a rule.

This feature is going to be reinstated on this PR. I did discuss it with @Vitalis95 so we will soon have it ready for testing.

@Vitalis95
Copy link
Contributor Author

@shadrackkibet, have a look at it.

@lloyddewit
Copy link
Contributor

@Vitalis95 Please could you resolve @N-thony 's comment above? thanks

rdstern
rdstern previously approved these changes Nov 4, 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.

@Vitalis95 to me this looks fine now. Let's see what @shadrackkibet thinks?

@N-thony
Copy link
Collaborator

N-thony commented Nov 5, 2021

Push a bit Name to label to the left, so that it aligns properly with other controls on the left side. Same case with New Data Frame Name label
image

@Vitalis95
Copy link
Contributor Author

@N-thony , I have made the alignments.

N-thony
N-thony previously approved these changes Nov 5, 2021
rdstern
rdstern previously approved these changes Nov 12, 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 good now.

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.

@Vitalis95 Thanks, this looks good, just a few suggestions

instat/dlgStack.vb Outdated Show resolved Hide resolved
instat/dlgStack.vb Outdated Show resolved Hide resolved
instat/dlgStack.vb Outdated Show resolved Hide resolved
instat/dlgStack.vb Outdated Show resolved Hide resolved
Co-authored-by: lloyddewit <57253949+lloyddewit@users.noreply.github.com>
@Vitalis95
Copy link
Contributor Author

@lloyddewit I have made the changes. Thanks

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.

@Vitalis95 thanks, looks good

@lloyddewit
Copy link
Contributor

@rdstern there's been some small changes since your last approval, please could you retest? 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.

Great!

@lloyddewit lloyddewit merged commit d85b7c5 into IDEMSInternational:master Nov 26, 2021
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.

None yet

5 participants