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

Implements notification for Levels/Labels #7625

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

derekagorhom
Copy link
Contributor

Fixes #7585
This is not ready for review yet.

@Patowhiz this is the pull request can we discuss how to make it more efficient.

@N-thony
Copy link
Collaborator

N-thony commented Jul 20, 2022

Fixes #7585 This is not ready for review yet.

@Patowhiz this is the pull request can we discuss how to make it more efficient.

@derekagorhom any progress with this?

@derekagorhom
Copy link
Contributor Author

Not yet @Patowhiz will check and suggest some modifications

@N-thony
Copy link
Collaborator

N-thony commented Nov 18, 2022

@derekagorhom how is it going here?

@rdstern
Copy link
Collaborator

rdstern commented Nov 27, 2022

This has been going since July and seems to have stopped. That's a shame, because the issue is a good idea. I would like to take this example to consider how we could each do better. But I start from my understanding of the issue itself, partly because I would like to raise a related point that might become a separate issue, but could usefully be considered at the same time that this issue is resolved.

As @Patowhiz says, to delete unused levels needs a separate dialogue - That's the Prepare > Factor > Remove Unused Levels dialogue. Here it is for an example with the survey data:

image

So, that's the task. Could we remove unused levels by adding the code and option from that dialogue, into the levels/labels dialogue. And it should presumably be quite easy, because the code already exists.

But, in the remove unused levels dialogue there is no option, because that is its job. In the Levels/Labels dialogue it should only be an option, because we don't need to complicate our lives with that point if there are no unused levels. And even if there are some unused levels, we may not automatically wish to remove them there.
a) So I propose a checkbox with that option, Label is Remove Unused Levels. Default is unchecked. If checked then it will remove them on Ok, but not if you just close the dialogue. I think that will be the same as in the Remove Unused Levels dialogue.
b) I propose it be invisible if there are unused levels.
c) (This may be for later, but I propose it becomes visible, but disabled, if there are unused levels and they cannot be removed.)
d) It is visible and the levels will be automatically removed on Ok is there are unused levels and they can be removed.

The new element I wish to add is another defect in the Levels/Labels dialogue and that is one that I hope we will also change soon? And I also have a question for @N-thony or @Patowhiz as to whether that improvement is easy?

We have a good system of filtering and most dialogues take account of a filter. The Levels/Labels does not. I understand why some other commands don't take account of a filter, but I can't see why it is a problem for the Levels/Labels dialogue. So could it also only show the unfiltered data, when a filter is in operation?

I would love that to be combined with the change suggested here. In that case it could be quite instructive to make the removing of unused levels visible, but disabled, when there are some that are unused, but that may be because of the filter. If so the checkbox is disabled and the message - like that in the Unused Levels dialogue when there are no unused levels would - this time say "Unused levels cannot be removed when data are filtered."

Now I come to the Levels/Labels dialogue. Here it is:

image

Now how would it change with this extra option? I suggest it is already deep enough, so we might make the grid part a little shorter, so there is room for that Green line to go underneath the other 2 controls. Then move them left and down a bit, so they line up and the green/red message comes under the checkbox. (If it is not too wide, then perhaps we can leave the grid as it is.

Now I question the delay of 5 months since @derek asked for help. Let's not try to find prople who were wrong, but let's learn from this.
@derekagorhom great that you managed to start this work. Fine that you asked for help. However, your request for help was not informative enough. It was a bit like someone saying "Doctor I am ill, please can you help me." without saying whether it is your head or foot that has a problem.
So, when asking for help, please be as specific as you can, to say where you have got to, and hence exactly where you want help.
@derekagorhom also be persistent, as @Patowhiz has a lot of varied work just now. So remind Patrick if you don't get a reply in a few days. Also check also with @N-thony if you don't get a quick reply, so he could also remind @Patowhiz or perhaps help you himself.
@N-thony great that you stepped in to check. Perhaps you could also have asked @derekagorhom what his problem was and asked him to write more details first?

And @derekagorhom I have also given a lot of general information above. Does that help you to proceed, or is it unrelated to the help you wanted?

@Patowhiz
Copy link
Contributor

@rdstern thank you for commenting on the possible solutions and explaining the different approaches articulately. I didn't think about the filtering aspects. Now that you have mentioned it, @derekagorhom this may not be as easy as I had thought. We will have to think about initial simple implementations first that factors presence of a filter.

@derekagorhom
Copy link
Contributor Author

@rdstern thank you for the suggestions, i will make this pull request a priority this week.

@rdstern
Copy link
Collaborator

rdstern commented Nov 27, 2022

@derekagorhom that would be ok, but you may wish to attempt #7953 first. There is quite a lot you can learn from that one and the recent changes to the summary keyboard have now been merged - so it is waiting for you.
In the summary and other keyboards for the calculator @anastasia-mbithe has done a very good job, and is willing to provide support if you need it.

@derekagorhom
Copy link
Contributor Author

Yes i will work on that too this week.
thanks

@N-thony
Copy link
Collaborator

N-thony commented Nov 21, 2023

Yes i will work on that too this week. thanks

@derekagorhom still working on this?

@N-thony
Copy link
Collaborator

N-thony commented Dec 18, 2023

@derekagorhom can you provide an update on the progress made here so far? Do you need help?

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.

Removing of unused levels in Levels/Labels dialog
4 participants