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

feature: Intellisense for Composer #3743

Merged
merged 19 commits into from
Aug 5, 2020

Conversation

LouisEugeneMSFT
Copy link
Contributor

@LouisEugeneMSFT LouisEugeneMSFT commented Jul 29, 2020

Description

Adding Intellisense to Composer textfields.

This PR includes:

  • a new language server (see commit 1)
  • a new ui textfield component with a hook that can communicate through LSP to the server (see commit 2)
  • integration of intellisense into Composer for expressions and for setting a property (see commit 3)

Task Item

fixes #3663
fixes #3664
fixes #3665

Screenshots

image

image

# Conflicts:
#	Composer/packages/extensions/adaptive-form/src/components/fields/index.ts
#	Composer/packages/extensions/adaptive-form/src/utils/resolveFieldWidget.ts
#	Composer/packages/ui-plugins/composer/package.json
@LouisEugeneMSFT LouisEugeneMSFT changed the title Intellisense for Composer feature: Intellisense for Composer Jul 29, 2020
@cwhitten
Copy link
Member

I love the useLanguageServer, this can accrue value anywhere we want to wire into the language servers.

What about useIntellisense(...) instead of <IntellisenseTextField />?

<IntellisenseTextField /> houses multiple useEffects and event listeners that I believe could be managed in a useIntellisense(...) hook, and I'd like to decouple this from the DOM if possible for a few reasons:

  1. As a developer, I'd much prefer to be able to use Fluent's docs directly with <TextField /> and other input controls and augment my code with useIntellisense(...) hook.
  2. Going with a hooks approach means that as you want to wrap new input controls with intellisense, you only need to pull in the hook instead of creating a new component that wraps the input control and copies the intellisense logic into it.

Going from:

<IntellisenseTextField {...} />
<IntellisenseInputControl2 {...} />
<IntellisenseInputControl3 {...} />
<IntellisenseInputControl4 {...} />
<IntellisenseInputControl5 {...} />

to

const [foo, setFoo] = useIntellisense(...)
<TextField {...} />

const [foo, setFoo] = useIntellisense(...)
<InputControl2 {...} />

const [foo, setFoo] = useIntellisense(...)
<InputControl3 {...} />

const [foo, setFoo] = useIntellisense(...)
<InputControl4 {...} />

const [foo, setFoo] = useIntellisense(...)
<InputControl5 />

@LouisEugeneMSFT
Copy link
Contributor Author

LouisEugeneMSFT commented Jul 29, 2020

Hi @cwhitten

There are a few challenges with this approach, I am curious to have your opinion on them:

  • The IntellisenseTextField component relies on some textfield specific props such as onKeyDown. Additionally everything is wrapped in a div that has its own handlers. All of that is necessary to react to keyboard events (such as navigating the list of suggestions using "up" and "down", selecting something using "Enter" or closing the list of suggestions using "Escape").
  • Additionally, having references to the components is necessary for click handlers (aka closing the list of suggestions if you click away from the textfield or one of the suggestions). Is is also useful in order to show the list of suggestions in the right place (although I see easy workarounds for that).

One thing I was considering for a more reusable component was to abstract away the fabric textfield, and take an extra optional input to overwrite a default un-opinionated textfield. Something like:

Happy to chat about those things in person

@LouisEugeneMSFT LouisEugeneMSFT changed the title feature: Intellisense for Composer feat: Intellisense for Composer Jul 31, 2020
@LouisEugeneMSFT LouisEugeneMSFT changed the title feat: Intellisense for Composer feat: Intellisense for Composer #3663 #3664 #3665 Jul 31, 2020
@LouisEugeneMSFT LouisEugeneMSFT changed the title feat: Intellisense for Composer #3663 #3664 #3665 feat #3663 #3664 #3665 Intellisense for Composer Jul 31, 2020
@LouisEugeneMSFT LouisEugeneMSFT changed the title feat #3663 #3664 #3665 Intellisense for Composer feature: #3663 #3664 #3665 Intellisense for Composer Jul 31, 2020
@coveralls
Copy link

coveralls commented Jul 31, 2020

Coverage Status

Coverage decreased (-0.3%) to 56.504% when pulling 5937679 on LouisEugeneMSFT:main into 5be55c6 on microsoft:main.

@LouisEugeneMSFT LouisEugeneMSFT changed the title feature: #3663 #3664 #3665 Intellisense for Composer Feature: Intellisense for Composer Jul 31, 2020
@LouisEugeneMSFT LouisEugeneMSFT changed the title Feature: Intellisense for Composer feature: Intellisense for Composer Jul 31, 2020
@LouisEugeneMSFT LouisEugeneMSFT merged commit 98b8e45 into microsoft:main Aug 5, 2020
lei9444 pushed a commit to lei9444/BotFramework-Composer-1 that referenced this pull request Jun 15, 2021
* Adding Intellisense server

* Adding Intelissense component

* Integrating Intellisense for "Properties" Setters and Expressions

* Merge update and dependency fix

* Lint issues

* pr comments

* PR comments (2)

* Adding id to textfield

* bug fix

* fixing require vs import

* Abstracting built in functions away from lg

* Fixing some bugs introduced after adressing PR comments

* Using render props

* Adding tests for intellisense server

* Adding tests for intellisense ui component

* Fixing an e2e test

* lint

Co-authored-by: Louis Eugene <leugene@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants