Desktop: Fixes #10060: Fix "New note" button rendering when startup with Trash can selected. #10076
Conversation
Previously, add note button is stored in useRef, which doesn't trigger rerender when ref change, change to useState instead.
|
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
|
I forgot to mention, I'm participating GSoC 2024. This is my introduction: https://discourse.joplinapp.org/t/introduction-duy-huynh/36448 |
| <StyledButton | ||
| ref={(el: Element) => { | ||
| props.setNewNoteButtonRef(el); | ||
| }} |
There was a problem hiding this comment.
Sorry I don't know why it works but this is not the right pattern to set references, so I'd rather not go with this change.
You also changed the logic to update the width or not, so maybe you fixed it this way and this change to the ref doesn't do anything
There was a problem hiding this comment.
Hi, thanks for your feedback. I read from here: render when ref change. It's a pattern here callback ref.
There was a problem hiding this comment.
Ref doesnt re render immediately as State, the ref inside useEffect hang on old value (I tested this, and this is how React intended useRef behavior).
To sum up: I want to use State instead of Ref, maybe my implementation is weird, I'd appreciate if you guide me some part of it, thank you
There was a problem hiding this comment.
I can prove that changing width logic doesn't fix the problem, but it's the state vs ref that fixes.
There was a problem hiding this comment.
If it works now I think it's only by chance, as a side effect of forcing a rendering when the state is updated.
It would be better to fully understand why it doesn't render. Usually it means that the effect is not taking into account a particular state variable. For example before it wasn't taking into account the width of the note list so that's why I've added this widthHasChanged check.
So I think the ref is fine but we need to find what part of the application state is not being taken into account.
There was a problem hiding this comment.
useRef version
This is total old version, I just log newNoteRef. You see, it hang on to old value.
joplinrefpoc1.mp4
And this is from React useRef() docs
My understanding: when switch to another tab, showNewNoteButton triggers useEffect, but newNoteRef stills hang on previous value. When newNoteRef change to new value, it doesn't trigger useEffect for re-rendering (because of useRef behavior from React docs above).
useState version
This is my useState version, every click to tab, run the useEffect twice.
2024-03-11.10-44-39.mp4
My understanding: when switch to another tab, showNewNoteButton triggers useEffect once, but newNoteRef stills hang on previous value (1st print is oldState). When newNoteRef change to new value, it triggers useEffect again (2nd print is newState). This keep the newNoteRef in sync with the updating breakpoints.
Also, the whole thing I do just switching tabs, no change width is involved.
| const [dynamicBreakpoints, setDynamicBreakpoints] = useState<Breakpoints>({ Sm: BaseBreakpoint.Sm, Md: BaseBreakpoint.Md, Lg: BaseBreakpoint.Lg, Xl: BaseBreakpoint.Xl }); | ||
| const previousWidth = usePrevious(width); | ||
| const widthHasChanged = width !== previousWidth; | ||
| const showNewNoteButton = selectedFolderId !== getTrashFolderId(); | ||
|
|
||
| // Initialize language-specific breakpoints | ||
| useEffect(() => { | ||
| if (!widthHasChanged) return; |
There was a problem hiding this comment.
Strictly speaking this check is correct and should not be removed. If the width hasn't changed, there's no reason to recalculate the breakpoints. But as mentioned maybe there's something else missing here.
There was a problem hiding this comment.
The case when width hasn't changed but need breakpoint calculations: when startup with Trash can selected (new note button hidden), then click on a note (new note button shown). No panel width changed but need to calculate breakpoint, or else the button will be big and overflow panel
There was a problem hiding this comment.
(new note button hidden), then click on a note (new note button shown).
So if this effect has a dependency on the button being hidden or shown, can this info be passed to it? As a result the effect should trigger again whenever the button visibility changes.
There was a problem hiding this comment.
Yes, the showNewNoteButton is a useEffect dependency already in previous versions. Yet, the problem is still useRef above, if useRef is null, even effect trigger, it cant update.
There was a problem hiding this comment.
Ok, fair enough, I feel there's something off with the implementation but the existing code wasn't great either so that might be related.
At least would you mind renaming your variables correctly please? You call them "ref" which implies they are React references but it's not what they are, they are HTML elements (you got the types right though). So please rename it to "newNoteButtonElement" for example.
|
Looks good now, thanks @khuongduy354 for looking into this! |
Summary
Previously, New note button ref is stored by useRef, which doesn't trigger rerender when ref change. This cause if the Trash can is selected in previous session, when reopen the app, the ref to New note is null by default, preventing the New note button to re-render into correct format.
More details and how to reproduce bugs: #10060.
Solution notes
So i remove this line
Testing