Note list does not render properly after searching for a note (Issue #4124)

I am working on this issue currently.

As @roman_r_m mentioned, this is caused by props.style.height being -50 when the sidebar for the list of notes in a notebook is toggled off. The negative value causes visibleItemCount and bottomItemIndex to become negative as well, which stops the list from rendering properly.

This does get fixed if we check when props.style.height becomes negative in visibleItemCount() and change it to,say, the maximum value possible which would be window.innerHeight. The accurate value will be strictly less than this. When the sidebar is toggled on, the re-render automatically corrects this to the actual value and gives the expected outcome. I implemented this just to check and it works fine.

This maybe a hacky fix, or not be a fix at all yet. I would need some advice on how to move forward from here.

1 Like

So first of all, I didn't do it but it may useful to figure out where this heigh of -50 come from, to understand if this itself is a bug or not.

I do not remember in details how this component works, but back then I had an idea that, maybe, the selected item index should be included in the component's state. Then it can be used to update all parameters correctly whenever needed.

Right now, some parameters are only calculated when the selected index is set and never updated when the component becomes visible.

Sure, I will figure out where the -50 comes from.

As of now, I think that the size prop reaches the <ItemList/> component (which is finally rendered as the list) when it is calculated using the itemSize() function in <ResizableLayout />, which in turn is called in the <MainScreen /> component. The searchbar is 50px in height (possibly from topRowHeight = 50 in the theme), which when subtracted for adjustment in itemSize() from 0 (i.e. when the sidebar is hidden), makes the value negative. But I'll definitely run through this once again.

Meanwhile, maybe @laurent could share any specifics regarding this ?

Wanted to share a few updates on this.

The -50 value seems to be coming from here (in <NoteListWrapper />)

const noteListSize = useMemo(() => {
		return {
			width: props.size.width,
			height: props.size.height - controlHeight,
		};
	}, [props.size, controlHeight]);	

When the list is toggled off, the height gets to zero and the controlHeight, which is 50px from theme.topRowHeight makes it negative. The original value of the height is computed in the layout handlers. The computed height should infact be zero when a component is not visible and thus I did not consider changing them just for this particular use case.

Now, I tried a bunch of different approaches to fix the height issue for this component. Here is what I could observe :

( <NoteListWrapper/> wraps <NoteList /> which in turn renders the final <ItemList /> )

  • Checking for negative height in <ItemList /> doesn't really work. Once the negative height is sent to the component the top, bottom, visibleItemCount and scrollTop indexes all are set to wrong values. When called again, even though the height value is corrected, these values remain wrong and the layout gets messed up in the new render. Their calculation is also dependent on one another so fixing them within the component seems infeasible.
  • Currently, toggling the list only updates <NoteList /> sizes . <ItemList /> properties are not changed and its size is passed directly with no updates in between the parent components.

As of now, to me the solution seems to be adding state related to the size(i.e height,width) in one of the above components, preferably the <NoteListWrapper /> (functional component) and when the list is toggled, restore the value of the height which prevents it from taking incorrect values. (similar to a componentWillReceiveProps, checking if the list is toggled using the visible prop and updating the height to the correct value ).

Something like :
(height is a state variable and previousVisible is the previous value of props.visible)

useEffect(() => {
		if (props.visible === previousVisible){
			setHeight(props.size.height - controlHeight);
		}
		else{
			//List toggled
			setHeight(previousHeight);
		}
	},[props.size.height,props.size.width,controlHeight]);

Since this is a functional component, the previous props are made available using a custom hook like this. I made an initial implementation and it seems to work for now. I'll discuss the complete implementation here if and when required.

@roman_r_m @laurent I wanted to know your thoughts on this before proceeding or opening a PR. Also , how should I go about testing this thing ?

Thanks!

To be honest, I already forgot the details of what's going on there. If you think this is the right way to fix it then just submit a PR.

As for testing, I know there are tests for some hooks but not sure how much of it will be applicable to your case. Obviously UI components are hard to test in unit tests.

1 Like

I've posted a PR to fix this issue. Kindly review it.