Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Display tags (params, returns...) along with method documentation #1523

Conversation

@jeremy-flusin
Copy link

jeremy-flusin commented Nov 22, 2019

  • All compiled assets are included

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

@lierdakil

This comment has been minimized.

Copy link
Collaborator

lierdakil commented Nov 23, 2019

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.

@jeremy-flusin

This comment has been minimized.

Copy link
Author

jeremy-flusin commented Nov 23, 2019

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.

@lierdakil

This comment has been minimized.

Copy link
Collaborator

lierdakil commented Nov 23, 2019

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.

@jeremy-flusin jeremy-flusin force-pushed the jeremy-flusin:1262_tooltips_for_params_and_return_doc branch from 1f8d6f4 to 8275fa6 Nov 25, 2019
@jeremy-flusin jeremy-flusin changed the title Display tags (params, returns...) along with method documentation WIP : Display tags (params, returns...) along with method documentation Nov 25, 2019
@jeremy-flusin jeremy-flusin force-pushed the jeremy-flusin:1262_tooltips_for_params_and_return_doc branch from 8275fa6 to aeab52b Nov 25, 2019
@jeremy-flusin

This comment has been minimized.

Copy link
Author

jeremy-flusin commented Nov 25, 2019

Okay I pushed some more commits, and now the two tooltips are displaying correctly.

tooltip1
tooltip2

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).
error

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 !

@lierdakil lierdakil force-pushed the jeremy-flusin:1262_tooltips_for_params_and_return_doc branch from 3918cfe to e08a005 Nov 25, 2019
@lierdakil

This comment has been minimized.

Copy link
Collaborator

lierdakil commented Nov 25, 2019

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.

@jeremy-flusin jeremy-flusin force-pushed the jeremy-flusin:1262_tooltips_for_params_and_return_doc branch from e08a005 to 3955efe Nov 25, 2019
@jeremy-flusin jeremy-flusin changed the title WIP : Display tags (params, returns...) along with method documentation Display tags (params, returns...) along with method documentation Nov 25, 2019
@jeremy-flusin

This comment has been minimized.

Copy link
Author

jeremy-flusin commented Nov 25, 2019

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 !

Copy link
Collaborator

lierdakil left a comment

A few more suggestions after looking at the code more closely.

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. It might seem a bit convoluted, but since both Etch and React will happily accept an array of elements in {}, rendering them in order, it would tuck most of the complexity under the rug, so to speak -- that is, into renderTooltip function. Overall I think this would lead to the cleaner code, but that is, of course, a matter of opinion.

lib/main/atom/tooltips/tooltipRenderer.tsx Outdated Show resolved Hide resolved
lib/main/atom/tooltips/tooltipRenderer.tsx Outdated Show resolved Hide resolved
lib/main/atom/tooltips/tooltipRenderer.tsx Outdated Show resolved Hide resolved
lib/main/atom/tooltips/tooltipView.tsx Outdated Show resolved Hide resolved
lib/main/atom/tooltips/tooltipView.tsx Outdated Show resolved Hide resolved
@jeremy-flusin

This comment has been minimized.

Copy link
Author

jeremy-flusin commented Nov 26, 2019

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.

@jeremy-flusin jeremy-flusin force-pushed the jeremy-flusin:1262_tooltips_for_params_and_return_doc branch from 07b6cd6 to 3d3ee83 Nov 26, 2019
@jeremy-flusin

This comment has been minimized.

Copy link
Author

jeremy-flusin commented Nov 27, 2019

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.

@jeremy-flusin jeremy-flusin force-pushed the jeremy-flusin:1262_tooltips_for_params_and_return_doc branch from c5ae9d2 to 6a6b24e Nov 27, 2019
@jeremy-flusin jeremy-flusin force-pushed the jeremy-flusin:1262_tooltips_for_params_and_return_doc branch from 6a6b24e to fbd9774 Nov 27, 2019
@lierdakil

This comment has been minimized.

Copy link
Collaborator

lierdakil commented Nov 27, 2019

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?

@jeremy-flusin

This comment has been minimized.

Copy link
Author

jeremy-flusin commented Dec 2, 2019

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?

Yep, absolutely. I didn't know a lot about the JSX.ElementClass lifecycle. Thanks for the suggestion. I just commited the suggested changes.

@lierdakil

This comment has been minimized.

Copy link
Collaborator

lierdakil commented Dec 5, 2019

Thanks for your work! Merging.

@lierdakil lierdakil merged commit 8b9a3d5 into TypeStrong:master Dec 5, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jeremy-flusin

This comment has been minimized.

Copy link
Author

jeremy-flusin commented Dec 5, 2019

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 !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.