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

Prevent setting an empty column name #5742

Merged
merged 3 commits into from
Nov 27, 2024

Conversation

boutinb
Copy link
Contributor

@boutinb boutinb commented Nov 26, 2024

@JorisGoosen
Copy link
Contributor

Also, to solve @tomtomme 's issue all you really need to do is call "delete column" properly if someone erases the name. Because that was the original intended behaviour right?
You erase the name to erase the column?

@tomtomme
Copy link
Member

Also, to solve @tomtomme 's issue all you really need to do is call "delete column" properly if someone erases the name. Because that was the original intended behaviour right? You erase the name to erase the column?

I would consider this dangerous behaviour IF the undo button does not work. Because it is not intuitive that deleting the name would delete also my data. Deleting the name is something a user most probably would do by accident. So the best intended behaviour from my point of view, would be, to restore the original name OR just do nothing.
If you disagree and think the column should be deleted on erasing the name please make sure that the UNDO button works. Thx!

@JorisGoosen
Copy link
Contributor

please make sure that the UNDO button works

Indeed, this is what I mean by "make sure it just calls deleteColumn" cause that would include the undo/redo

@boutinb
Copy link
Contributor Author

boutinb commented Nov 27, 2024

But I also disagree that making the name empty should remove the column: most of the time, making the name empty will be just a bad manip of the user. Trying to make it empty should reset the old name.

@JorisGoosen
Copy link
Contributor

Ok wahtever but Im fairly sure you @boutinb are the one who added this functionality in the first place.

Copy link
Contributor

@JorisGoosen JorisGoosen left a comment

Choose a reason for hiding this comment

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

Ill approve it now, despite it being very ugly that the name stays empty:
image

@JorisGoosen JorisGoosen merged commit aae799b into jasp-stats:development Nov 27, 2024
1 check failed
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.

4 participants