-
Notifications
You must be signed in to change notification settings - Fork 35
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
Harmonize dialogs by adding apply, cancel, and reset buttons #265
Conversation
Thanks for the first implementation. I ran a first quick test and it looks already very promising. I found a couple of things that needs to be addressed.
|
# Conflicts: # src/renderer/HistogramCellRenderer.ts
re 1: done re 2: when I try https://lineup.js.org/app_develop/#soccer it behaves the same. just clicking outside will close all dialogs. A proper dialog ending (cancel, apply) won't close the more dialog |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-
Thanks, that looks already better. I have just a minor typo in the last commit.
-
You are right. Then let's keep it like this for now. I will discuss this behavior with the others, if we want to change it.
which instance should trigger those events? it cannot be part of any model (column, ranking, data provider) since it is purely UI related |
What about the LineUp instance itself? Alternatively you could add a dedicated event dispatcher for UI-releated events. |
Thanks for fixing #277 and implementing the events. With this addition we should be able to filter out the live preview events. In general it works now very smooth. While testing I just found two bugs: 1. Reset does not work for missing value rows in categorical column filter
Nothing happens. As I user, I would expect that the checkbox is unchecked on reset. 2. Cannot filter missing values in numbers column
There are no missing value rows found, even though there are clearly some in the dataset. As I user, I would expect that the checkbox works and I can filter out missing values rows. |
done |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that looks good now. Thanks for implementing and unifying all of the dialogs which is a huge usability improvement! I'm going to merge the PR. If we find further bugs, we will open separate issues for it.
Check if PR might cause the build error lineupjs/lineupjs#265
Check state before merging PR lineupjs/lineupjs#265
closes #229, #272, #277
prerequisites:
Summary
unifies the dialogs as described in the issue