Open nrc opened 6 years ago
Has to be a ref menu, not a line number menu. Closing the menu also jumps
This is because we calling componentDidUpdate
when opening or closing a ref menu which calls jumpToLine
@nrc is componentDidUpdate
being called when opening or closing a ref menu due to refMenu
being in the state? Or is there somewhere explicit that's being called?
I'm also wondering what the reason is for calling componentDidUpdate
from within componentDidMount
- is it to initialize the source links?
is componentDidUpdate being called when opening or closing a ref menu due to refMenu being in the state?
Yes
I'm also wondering what the reason is for calling componentDidUpdate from within componentDidMount - is it to initialize the source links?
That and to add any highlights and to scroll to the correct line.
If we were to do this 100% properly, then the highlights should be a component and could be handled implicitly via render
, and the source links too could probably be done inline. However, that is (I think) a big refactoring. I'm not sure about the best way to handle the scrolling, I think maybe DidUpdate and DidMount is the right place for that.
Given the constraints, I'm leaning towards adding a flag for whether or not the refMenu
has changed in state, within componentDidUpdate
. It would be added to the this.props.highlight
conditional to determine whether or not jumpToLine
should be called. Does that sound too clunky?
I was trying to think through ways to better tap into the component lifecycle, but not having the highlights as a component I think means we have to keep those conditional checks and behavior within componentDidUpdate
.
So f22b6f798ea9e870d3fdf2346caedd16724e9a86 is what I had in mind for adding a menuChanged
flag, which would be used to determine whether or not jumpToLine
should be called. It seems to solve the immediate issue, but there continue to be conflicts down the line with this.props.highlight
and various components updating.
For example, if you open and close a menu, it should be fine, but if you then click on one of the src_link
items, you'll jump to the previously set line_start
. I'm not sure of the best way to handle this, since the highlight
prop is set on a parent component.
Also, I'm not sure why that commit ID above is linking to the main repo instead of my fork, but I've got a branch w/ that commit on it in my fork ¯_(ツ)_/¯
How much work do you think it would be to factor out a highlight component? That seems like the 'right' thing to do here. I have shied away from doing it in the past just because it seemed hard, but maybe it is not too bad?
It might be possible to keep the line to jump to in the state rather than just a prop, then when we use set state to open a menu we also remove the line to jump to. Not sure exactly when we'd initialise the state from the prop though.
wrt a highlight component - I think it would be worth putting some time against, at least. I remember thinking at some point that there would be some additional issues that would be easier to address if highlights were their own component. I should be able to look into it more later this week, or this weekend.
Hi @nrc - I've been spending some time working on a highlight component, and I have a proof-of-concept working for a very basic case of a highlight in a single line.
What do you think is a good way to test multi-line highlights? I'd like to make sure I'm testing against a few different highlight cases, then see if this helps with the jumpToLine
issues.
Hmm, I'm not sure we even use the multi-line highlights any more. They were used for error highlights, but with search and navigation, I think we only highlight single lines.
Ah, cool. In that case, I'll leave in the logic just in case it's still in use somewhere we forgot, but focus on testing with single-line highlights.
STR: