Improve how the findbar's toggle button is referenced in the viewer code #18510
Conversation
The sidebar and secondary toolbar both have a reference to their toggle buttons in their own sections in `getViewerConfiguration`, so it makes sense for the findbar to do the same. While we actually have a findbar-specific reference to the toggle button, I noticed that we don't use it consistently because the toolbar also has a reference to the exact same toggle button and we use both in the code. This is probably for historical reasons: the docstring in the toolbar file indicates that the `viewFind` element is an input to the component, but that option is never actually used in the code itself. This commit fixes the issue by removing the toolbar-specific reference, since it's not actually used (anymore) in the toolbar code, so that we consistently use the findbar-specific reference everywhere.
|
/botio-linux preview |
From: Bot.io (Linux m4)ReceivedCommand cmd_preview from @timvandermeij received. Current queue size: 0 Live output at: http://54.241.84.105:8877/5839299d2eddb22/output.txt |
From: Bot.io (Linux m4)SuccessFull output at http://54.241.84.105:8877/5839299d2eddb22/output.txt Total script time: 1.04 mins Published |
|
I found this during a review of #18385 where I stumbled upon the following line not having been changed to Line 52 in 4484fde I found that it's quite easy to overlook such issues during review, and in this case it'd cause a subtle bug that we wouldn't immediately have found (since it's only visible in the To investigate how feasible it is to extract smaller chunks out of #18385 I figured I should first get this pre-existing issue fixed to simplify the code and, if accepted, I can follow this up with PRs to extract actual bits and pieces from the PR (obviously crediting @calixteman as co-author of the work). Hopefully that'll help to make some progress on integrating the nice work from #18385 into the codebase in smaller steps, and get rid of some viewer code legacy along the way. |
The sidebar and secondary toolbar both have a reference to their toggle buttons in their own sections in
getViewerConfiguration, so it makes sense for the findbar to do the same.While we actually have a findbar-specific reference to the toggle button, I noticed that we don't use it consistently because the toolbar also has a reference to the exact same toggle button and we use both in the code. This is probably for historical reasons: the docstring in the toolbar file indicates that the
viewFindelement is an input to the component, but that option is never actually used in the code itself.This commit fixes the issue by removing the toolbar-specific reference, since it's not actually used (anymore) in the toolbar code, so that we consistently use the findbar-specific reference everywhere.