Closed jeremy-flusin closed 4 years ago
Hi. Thanks for creating the pull request! I have a few suggestions, which I will outline shortly as diff comments, but a couple I do have to note here.
Atom-TypeScript can work without atom-ide-ui also, in which case it needs to use builtin tooltips. The code for those is in lib/main/atom/tooltips/tooltipView.tsx
, particularly in the tooltipContents
method. Do you suppose you could do pretty much the same there? Yes, I know, a bit of code duplication is happening here. If you'd like to avoid that, feel free to re-use TSDatatipProvider
, or factor out the common code.
The reason for using class
is in part historical, but the crux of the matter is: we're not actually using React, and in etch class
and className
are in fact synonyms. So for the sake of consistency, I would request you to use class
actually. It's probably a reasonable idea to switch to className
at some point, but we should do that on the whole code base at once, and that probably should be a separate PR.
Hi ! Thank you for your comments. Be sure I'll add commits to my PR accordingly as soon as I can (probably early next week).
About moving class
into className
tags, I totally understand the benefits of a dedicated PR. I'll revert these modifications. It's just that an error popped in the console everytime I tested the tooltips, so I thought that it'd be a good idea to fix it while I was at it.
an error popped in the console everytime I tested the tooltips
Unless you're using some React-specific plugins, it really shouldn't have... I mean, etch doesn't complain about class
at all. So I'm not entirely sure what you're referring to.
Okay I pushed some more commits, and now the two tooltips are displaying correctly.
I only managed to factor out the code for the two tooltip renderers by passing the etch
instance to the builder method. I assume that's because the one in TSDatatipProvider
is kinda mocked or overriden ? I'm faily new to these libraries so I may have done something stupid that could have been avoided. Let me know if I did, haha.
Concerning the error I mentioned the other day, here it is. It pops everytime I make a tooltip appear (even if I checkout to master).
I just realized that I made a little regression on the first tooltip (I made the signature method disappear). I'll fix that along with the modifications you may suggest, hence the WIP status on the PR.
EDIT: fixed
Cheers !
I only managed to factor out the code for the two tooltip renderers by passing the etch instance to the builder method
Yeah, I kinda forgot about this quirk. So here's the thing: we use etch, and atom-ide uses React. Atom-ide accepts either markdown, which doesn't work for us, or React components. But we don't really want to depend on React directly (it's big and heavy), and we still want to use etch. Hence, a minimal React JSX constructor is mocked in lines 7-26 of datatipProvider.tsx
. That is also why you're seeing the error thing -- sorry I didn't realize this sooner.
Concerning the error I mentioned the other day
Yeah... It only pops up in Atom's dev-mode, which I rarely use, so it went unnoticed for a while. Honestly, not a huge deal, React complains, but works fine. Anyway, I've pushed a commit to master
that changes class
to className
project-wise. I took the liberty of rebasing this PR on current master
. Please remember to fetch & rebase your local changes before trying to push.
Okay, I understand better what's going on under the hood. Let me know if you want me to change anything, otherwise I'll let you merge this.
Cheers !
One thing I have to note is that I would probably go one step further with
renderTooltip
and let it render the code, too -- just pass it the code rendering function as well.
Agreed, and this was my first thought too. The problem is that one tooltip is renderered fully synchronously, and the other, because of the highlightCode
part, is asynchronous. I suppose that it's because of all of the atom
calls ? I just couldn't figure how to bypass this problem. Any idea welcome !
Anyway, I fixed pretty much all the suggestions you had earlier. I will commit them very soon.
I added one more commit. I now make renderTooltip
render the code part too, and just don't use the result in the datatipProvider, because of the asynchronous calls we have to await for code highlighting.
This feels somewhat redundant... Why not make renderTooltip
async and move its invocation to update
method in TooltipView
? You'll need to save its result though, but it's nothing a private class property can't handle. Does that make sense?
This feels somewhat redundant... Why not make
renderTooltip
async and move its invocation toupdate
method inTooltipView
? You'll need to save its result though, but it's nothing a private class property can't handle. Does that make sense?
Yep, absolutely. I didn't know a lot about the JSX.ElementClass lifecycle. Thanks for the suggestion. I just commited the suggested changes.
Thanks for your work! Merging.
Thanks for your work! Merging.
Hey, no problem ! Thank you for your time & help on this cool projet. I'll try to take an issue from time to time.
Bye !
I tried something to solve #1262 Also renamed some 'class' tags to 'className' as recommended by React
Also, this is my first contribution. Feel free to amend & comment.
Preview:![first_contrib](https://user-images.githubusercontent.com/5628536/69439489-61086280-0d47-11ea-9205-929c850f2ebd.png)