Issue 2395/misleading with create person very high in select widget

[xela1601]

Description

This PR closes #2395

Changes

• Fixed issue about "Create person" button in select-widget, so it is only showing that button if a search has actually been performed before
• Changed placeholder text in select-widget for calling-assignments, to make it more clear that a search has to be performed by typing

Notes to reviewer

Within the caller-assignment page:

• open the dropdown => button is not there
• start typing at least 3 letters => searching persons from db should be triggered => button appears
• add a person that has been loaded from the database after typing at least 3 letters => the button should disappear again
• add a new person (click the "add person"-button, fill out required fields and submit) => the button should disappear again
• attempt to add a new person and cancel => the button should disappear

[richardolsson]

Really nice work getting into this! I've left some comments below and would love to hear what you think.

The language files should not be touched by you, except maybe to delete the obsolete translations. This is not obvious, I know!

[richardolsson]

If I'm not mistaken, wasLoading is synchronized with autoCompleteProps.isLoading, by which I mean to say that whenever isLoading becomes truthy, wasLoading is set to true, and whenever isLoading becomes falsy, wasLoading is set to false.

The name wasLoading to me implies that it should remain true after the loading completes. Right now, it is being reset to false after loading finishes, and instead showAddPersonButton becomes true.

This makes me wonder whether two states are really necessary, or whether one could be derived from the other. If wasLoading? could be set to true when loading happens, and only reset once a person is selected, then I would expect showAddPersonButton` to not be necessary. Am I missing something?

[xela1601]

i finally had the time to look into this and yes you are correct.
Can you have a look again? 😅

[richardolsson]

To follow up on my previous comment, what if this used wasLoading instead (and wasLoading was not set to false on line 215).

Suggested change

	{!disabled && showAddPersonButton && (
	{!disabled && wasLoading && (

[xela1601]

✅

[richardolsson]

Instead of changing this, you should change the message in messageIds.ts and remove the translation from en.yml and all other YAML files.

[xela1601]

-> b4fb79d

[richardolsson]

Great work @xela1601! This is working as expected and I think it can be merged as is. I will approve and merge!

I will also separately take this opportunity to share my thoughts about code comments, which is a topic I often return to, but never find the time to fully exhaust. I think this code provides a great example around which I can reason about comments, and I might link to this in the future. 😊

First of all, let's get something important out of the way. I judge comments based on two things that I consider facts. If you (the reader) believe I'm wrong about these, that defeats my entire argument, so I will state them very clearly up front. I believe (A) that the purpose of comments is to make code more understandable, and I also believe (B) that most programmers do not read comments unless they feel the need to, i.e when they struggle to understand the code.

This means that a lot of times, a person who edits code (and then hopefully understands it) will not read the comments, and one of the most common mistakes that I see (and often don't see because it's so easy to miss) is that code gets updated without the relevant comments getting updated alongside it, leading to comments that contradict the code they're commenting on.

When comments contradict the code they comment on, that's causing (way) more harm than good. In those cases, future readers will probably be confused rather than helped by the comments, and they would be better off just reading the code without any comments.

For me, the risk of that happening is big enough (I've seen it very often from both juniors and seniors, including myself), and the consequences bad enough (confusion) that I do my best to avoid ever having comments. That's right, I encourage everyone to not include comments in their code! 😱

If code needs comments, I find that most of the time it can be made more understandable by changing the structure of the code. Especially, I find that changing names of existing functions and variables, or introducing new functions and variables that can be named, is often enough.

Let's investigate how that could be done here.

[At this point, I advice you to read my comments on different parts of the code in this PR, and then return to read the final part below].

Finally, let me be very clear that I'm not saying that all of these extracted functions and long variable names (that I have used as examples below) are perfect or even objectively better than the comments. But I'm claiming that they communicate (roughly) as much as the comments, without any comments, and that the risk of changing the code and forgetting to change the names of variables/functions is less than the risk of changing the code and forgetting to change the comments.

In situations like this, I try to make code so readable that comments are not necessary, and my suggestions below is one way of doing that, although in reality it's probably not even necessary in this situation.

This PR is quite small, which makes it a great example, but that also means that my suggestions are a little bit over-complicated. In this specific example, I would personally probably have just removed all the comments, and been happy with the code as is after that. 😊

I appreciate the space to share these thoughts, but again, they are my personal thoughts and not something that needs to prevent this PR from being merged. 💯

[richardolsson]

Starting from my general thoughts around code comments, here are my thoughts on specifically how this comment could have been communicated through the code instead.

I think that the first line of the comment is unnecessary, because it is stating obvious things. The first is that the code below is [setting] wasLoading to true (which can already be read from the name setWasLoading(true)). The second is that this happens when the autocomplete is loading (which is obvious from the names in the expression in the if statement).

I think the second line of the comment however does provide some additional information, which could be helpful in understanding the lifecycle of this state. I don't think it's strictly needed to understand this from just this one line of code, but for the sake of this exercise, let's try to communicate that knowledge in the code alone.

One way to do this would be to change the name of the state, from wasLoading / setWasLoading to hasLoadedSinceLastSelect / setHasLoadedSinceLastSelect.

Granted, this makes the name very long, and I would weight that against the need for this level of clarity (and probably arrive at the conclusion that it's not necessary) but it's still no longer than the comment, and it's much less likely that a coder makes changes to this variable that changes how it works, but forgets to change the name. The comment is more likely to "rot" as time passes and code changes.

[richardolsson]

Starting from my general thoughts around code comments, here are my thoughts on specifically how this comment could have been communicated through the code instead.

This comment communicates several things. Again, I'm not sure any of those things really need to be communicated, because I don't think they are super complicated, but lets say they were, and we could only remove the comment if the same meaning was expressed as well or better in the code.

First, the comment tells us that this is happening when a person is selected. In some other context, it could be argued that this is only necessary because the onChange name is not clear enough (imagine if it was called onPersonSelect instead) but in this case we can't rename it because the MUIAutocomplete is general-purpose.

But we can name the callback that this code exists in, i.e. the currently unnamed function defined right here. If it was defined higher up in this component and called something like onPersonSelect or handlePersonSelect, it would be more clear:

function handlePersonSelect(ev: unknown, person: ZetkinPerson) => {
  // …
}

Creating a function allows us to name a concept. Until a few years ago, I've often thought of functions as something we use in order to reuse code or extract code away. But another important aspect of functions is that they allow us to name the thing that happens in it. That can help communicate the intent of code, the way handlePersonSelect does here.

Then this could be used in the JSX, and it communicates both there and up where it's declared what the context is (a person being selected):

Suggested change

	onChange={(ev, value) => {
	onChange(ev, value);
	// If a person is selected, we reset the wasLoading state to false
	// A new search has to be done to show the add person button again
	setWasLoading(false);
	}}
	onChange={handlePersonSelect}

Second, the comment tells us that the code will reset the wasLoading state to false. I would argue that this is sufficiently communicated already by the code as is. The only aspect of that statement that is not obvious in the code setWasLoading(false) is that this is considered a "reset" rather than just a "set". If we want to communicate this, we could create a function called resetWasLoading and invoke that:

function resetWasLoading() {
  setWasLoading(false);
}

Finally, the comment tells us that a new search has to be done to show the add person button again. Again, I'm not sure that this is necessary to convey here, but if we wanted to, the new name for the state (invented in my separate review comment) would hopefully convey this.

function resetWasLoading() {
  setHasLoadedSinceLastSelect(false);
}

[richardolsson]

Starting from my general thoughts around code comments, here are my thoughts on specifically how this comment could have been communicated through the code instead.

This code (and the comment) is very similar to the one from onChange on the MUIAutocomplete above, and I would treat it the same way.

The first part of the comment tells us that this happens [when] a person is added. This is somewhat misleading, because this code is not executed when the person is added, but when the dialog is opened. But either could be communicated by creating a separate function.

function handleCreateDialogOpened() {
  setCreatePersonOpen(true);
  setWasLoading(false);
}

The remainder of the comment tells us that the code will reset the wasLoading state to false and that a new search has to be done to show the add person button again. We have already succeeded in communicating both of these using solutions in my previous comment.