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

New Data Frame #4971

Merged
merged 55 commits into from
Oct 29, 2018
Merged

New Data Frame #4971

merged 55 commits into from
Oct 29, 2018

Conversation

Patowhiz
Copy link
Contributor

@Patowhiz Patowhiz commented Sep 21, 2018

Fixes #4439 Extending the File > New dialogue even more.
@africanmathsinitiative/developers this is ready for review

@Patowhiz
Copy link
Contributor Author

@africanmathsinitiative/developers this is now ready for review

@maxwellfundi
Copy link
Contributor

@Muthenya @JacklineKemboi @shadrackkibet can someone test this please?

@Patowhiz
Copy link
Contributor Author

Patowhiz commented Oct 9, 2018

Should we have yellow for incomplete columns and red for columns with an error?

@dannyparsons
Copy link
Contributor

It depends what you mean by error. An error in the R command (which we want to implement later) could have just the cell highlighted in red.
It looks like you are using red for duplicate column names. I think that's different and I thought we were going to require unique names by not being able to leave the cell if there's already a column with that name. I prefer this because its not obvious that this is the problem if its just highlighted.

@Patowhiz
Copy link
Contributor Author

Patowhiz commented Oct 9, 2018

We have a message box outlining this. It tells the user that there's a duplicate. I don't like preventing the user from leaving a control. It goes against the rules of a good UX. For instance the user could want to correct the other cell not the current selected one

@dannyparsons
Copy link
Contributor

Ok that sounds fine. Then I think we have it highlighted in yellow too, and reserve red for when we check errors in R code.

@Patowhiz
Copy link
Contributor Author

Patowhiz commented Oct 9, 2018

Okay. We will have red for invalid column name and invalid expression

@Patowhiz
Copy link
Contributor Author

Patowhiz commented Oct 9, 2018

Invalid column name to mean it's a reserved R keyword

@shadrackkibet
Copy link
Collaborator

Testing this again.

@shadrackkibet
Copy link
Collaborator

I have tested this and it now works better. @dannyparsons this is ready.

@dannyparsons
Copy link
Contributor

I'm getting a few inconsistencies here. Firstly, it only shows one incorrect cell at a time. When I had multiple gaps only the first one was highlighted in yellow. It would be good to check all cell and not stop when an error is found. Because of this, I could for example, type an invalid column name, but it didn't show as an error because an earlier cell was empty.
I tried TRUE as a column name and it correctly told me that wasn't allowed. When I corrected it to a different name it stayed red when there was a previous cell highlighted in yellow. I think this is the same cause as above where it stops checking after the first incorrect value.

It's good that OK was always disabled when I had something wrong. It never let me click ok when rows were incomplete or had invalid/duplicate names.

@Patowhiz
Copy link
Contributor Author

@dannyparsons should we then remove the message boxes? they will be annoying if that's the case

@dannyparsons
Copy link
Contributor

Yes we probably don't want multiple message boxes appears each time a cell in changed.

@Patowhiz
Copy link
Contributor Author

And how will the user know what S/he is doing wrong?

@dannyparsons
Copy link
Contributor

I was going to suggest a tooltip on the problem cells, but that might be too much work.

@Patowhiz
Copy link
Contributor Author

Lets just leave the messageboxes then.

@dannyparsons
Copy link
Contributor

Sounds fine.

@Patowhiz
Copy link
Contributor Author

@dannyparsons this is ready for your reveiw

@Patowhiz
Copy link
Contributor Author

@dannyparsons this is ready for your review

2 similar comments
@Patowhiz
Copy link
Contributor Author

@dannyparsons this is ready for your review

@Patowhiz
Copy link
Contributor Author

@dannyparsons this is ready for your review

@dannyparsons
Copy link
Contributor

This is ready other than the 2 spacing issues.

@Patowhiz
Copy link
Contributor Author

@dannyparsons changes applied

@dannyparsons dannyparsons merged commit f008f37 into IDEMSInternational:master Oct 29, 2018
@Patowhiz Patowhiz deleted the FileNew branch November 12, 2018 08:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants