Skip to content
This repository has been archived by the owner on Sep 6, 2021. It is now read-only.

Type Inference in hint list. #11949

Merged
merged 14 commits into from Dec 10, 2015
Merged

Type Inference in hint list. #11949

merged 14 commits into from Dec 10, 2015

Conversation

swmitra
Copy link
Collaborator

@swmitra swmitra commented Nov 23, 2015

Show type data in the hint list. This is work in progress as we need more ideas about the type symbols which are just text in this PR.
types

A snapshot of the rendered types from this implementation.

@swmitra
Copy link
Collaborator Author

swmitra commented Nov 23, 2015

Tagging @nethip @abose @ryanstewart

@swmitra swmitra added this to the Release 1.6 milestone Nov 23, 2015
@swmitra swmitra self-assigned this Nov 23, 2015
@sprintr
Copy link
Contributor

sprintr commented Nov 23, 2015

I believe it would be nice to show the data types as the output of typeof(instance) such as "string", "number", "function".

Edit: We can also use "keyword" to show type of keywords.

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Data_structures

@swmitra
Copy link
Collaborator Author

swmitra commented Nov 23, 2015

Thanks @sprintr. I will try that once and post a snapshot of the rendering. Our goal is to have the type data without being too loud. We have to compare different displays in order to conclude.

@swmitra
Copy link
Collaborator Author

swmitra commented Nov 24, 2015

@sprintr With some CSS changes and new literals , this is how it looks.

types1

I must confess , to me it looks cluttered.

@petetnt
Copy link
Collaborator

petetnt commented Nov 24, 2015

@swmitra maybe aligning the literal labels to right would help (plus maybe more padding to the right side). I think that the labels the OP look pretty nice too, but then again the literals are more clearer (in a way). Maybe the labels could come from a labelProvider for a customization option 🍻

What about mixed types? For example [0, "string", {object: "foo"}]

@abose
Copy link
Contributor

abose commented Nov 24, 2015

+1 for right align.
+1 for short symbols as in the initial implementaion. Maybe a hybrid- on hover/arrow key on current selected entity, we could expand the short symbol to detailed string?

@ficristo
Copy link
Collaborator

FWIW, I like the second screenshot with right alignment.
I looked a bit in VisualStudioCode which has the inference type too: I like it can inference the return type of a function too.

@swmitra
Copy link
Collaborator Author

swmitra commented Nov 24, 2015

@petetnt @ficristo @abose @sprintr A snap with right aligned symbols and more padding.
types2

@swmitra
Copy link
Collaborator Author

swmitra commented Nov 24, 2015

@petetnt I will write a label provider which can be overridden by extensions.
autocomplete plus works in the same way I guess.
Also I have to infer a mixed type array , will update the PR with the check.

@ficristo
Copy link
Collaborator

@swmitra Can you try to put the type on the right of the variable names?

@abose
Copy link
Contributor

abose commented Nov 24, 2015

😅 I too meant right align after the variable names!

@sprintr
Copy link
Contributor

sprintr commented Nov 24, 2015

This looks good. I am not sure if short names will look good in right alignment.
+1 for right alignment.

@swmitra
Copy link
Collaborator Author

swmitra commented Nov 24, 2015

@sprintr @petetnt @ficristo @nethip Was trying with different styles. This one
types3
stands out with a small css tweak.

@swmitra
Copy link
Collaborator Author

swmitra commented Nov 24, 2015

@larz0 @ryanstewart Please provide your inputs.

@swmitra
Copy link
Collaborator Author

swmitra commented Nov 24, 2015

Some more tuning with the type details ( font-style and weight change )
types4

@ficristo
Copy link
Collaborator

Just throwing a couple of ideas:

  • the icon and the type should gray and only the one selected ones should be blue so the focus is on the autocomplete (or simply found some way to make the autocomplete a bit more outstanding)
  • the icons seem ot use another style but they are interesting, maybe make them use less space
  • I don't like much the italic version but the word keyword look better there. In the previous screen it has a too much different style

@swmitra great job 👍

@swmitra
Copy link
Collaborator Author

swmitra commented Nov 24, 2015

Thanks @ficristo . Something like this -
types5

@ficristo
Copy link
Collaborator

As usual my english need some improvements...
I like the smaller icons but I meant that the left padding should equal to the right: so reduce the right padding. Then the icon will be centered and it will use less space.
The keywords should remain in blue so they stand out more.

And then I stop here.
@swmitra Thank you for your the effort.

@sprintr
Copy link
Contributor

sprintr commented Nov 24, 2015

I think the function argument list can be omitted from the code hints (see var3 in the above screenshot) since brackets already supports it. This will make code hints small and tidy.

2015-11-24 19_40_48- main js html - brackets

@swmitra
Copy link
Collaborator Author

swmitra commented Nov 24, 2015

Thanks a lot @sprintr for your prompt feedback. I agree that it doesn't exactly look tidy , but from a developer point of view , I think this adds more flexibility even before I choose the guess. In it's current form, the hinting module allows the parameters hinting only after choosing the guess from the list. But in the approach above we get to see the param type information upfront.

I know it's debatable , but that's why communities are best. In a day the entire ux got changed and became considerably more informative.

@swmitra
Copy link
Collaborator Author

swmitra commented Nov 25, 2015

Snapshots under default dark theme and light theme.
types6dark
types6

@sprintr
Copy link
Contributor

sprintr commented Nov 25, 2015

@swmitra Maybe we could take a leaf out of pref code hints where lengthy descriptions are shown under the hint. The type could come on the right and the function signature could come under the hint.

@swmitra
Copy link
Collaborator Author

swmitra commented Nov 25, 2015

@sprintr I was actually planning to use that for available docs for the particular definition when highlighted.

@abose
Copy link
Contributor

abose commented Dec 8, 2015

Tagging @larz0 for ui review.

@swmitra
Copy link
Collaborator Author

swmitra commented Dec 8, 2015

Uploading new screenshots for brackets light and dark theme.
latest_light
latest_dark

'<>' in Red indicates an external link for more details. Fetched from JS doc.

@swmitra
Copy link
Collaborator Author

swmitra commented Dec 9, 2015

@sprintr @petetnt @ryanstewart @abose @ficristo Feedback Please 💬

display: none;
padding-left: 28px !important;
padding-right: 10px !important;
color: grey;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't look very visible in the dark mode. We use #ccc in pref hints so it will be good to use it here as well.

Dark Mode: #cccccc
Light Mode: #6e6e64

@sprintr
Copy link
Contributor

sprintr commented Dec 9, 2015

Hints with lengthier signature are breaking the layout of individual hints.

2015-12-09 16_00_14- main js html - brackets
2015-12-09 16_08_51- main js html - brackets

@swmitra
Copy link
Collaborator Author

swmitra commented Dec 9, 2015

@sprintr It is intentional to make the type details appear in next line if it is beyond a certain character length.

@abose
Copy link
Contributor

abose commented Dec 9, 2015

It will look better if the signature is also right aligned in the second line.

@sprintr
Copy link
Contributor

sprintr commented Dec 9, 2015

Maybe we could remove the 5px padding on the signature then.

@sprintr
Copy link
Contributor

sprintr commented Dec 9, 2015

// 1
function Get() {
    var h1 = document.
}
// 2
function Get() {
    var h1 = document.crea
}

Brackets now throws an error and doesn't give any code hints for script 1 (from dot up to 3 chars) whereas script 2 works just fine.

@swmitra
Copy link
Collaborator Author

swmitra commented Dec 9, 2015

@sprintr Fixed now. jQuery was complaining as it was thinking that js doc spans innerHtml was malformed ( as it contains html tags sometimes ). Fixed that by adding the doc as text instead of html.

@sprintr
Copy link
Contributor

sprintr commented Dec 9, 2015

@swmitra Great. works fine now.

@swmitra
Copy link
Collaborator Author

swmitra commented Dec 10, 2015

@sprintr Replaced '<>' with a boxed '?'. Looked better to me. Please have a look and let me know.

@sprintr
Copy link
Contributor

sprintr commented Dec 10, 2015

@swmitra Looks better to me too. <> had an html like feeling to it.

abose added a commit that referenced this pull request Dec 10, 2015
@abose abose merged commit 60eaca4 into master Dec 10, 2015
@abose abose deleted the swmitra/TypeInferenceInHintlist branch December 10, 2015 12:11
@abose
Copy link
Contributor

abose commented Dec 10, 2015

Thanks @swmitra + @petetnt @ficristo @sprintr @Denisov21 For this feature 🎆
All unit tests/ extension tests for code hints passing.
Tagging @larz0 for UI Review.
Please log issues if any found for the feature.

Merging to give better testing window before shipping.

@nethip
Copy link
Contributor

nethip commented Dec 10, 2015

@swmitra Great work! This is awesome.

Small recommendation. I think the hints list looks cleaner without the box icons on the left. Also we could do something better with replacing current icon "?". Let us go without the box icons for now. We could then incorporate the changes(or icons) once we hear from @larz0 .

@ex1st
Copy link

ex1st commented Jan 18, 2016

Map and Set have been forgotten.

@swmitra swmitra changed the title [WIP]Type Inference in hint list. Type Inference in hint list. Apr 5, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants