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

Events not consistently passed to wrapped React components #289

Closed
mattrothenberg opened this issue Nov 3, 2021 · 11 comments · Fixed by #308
Closed

Events not consistently passed to wrapped React components #289

mattrothenberg opened this issue Nov 3, 2021 · 11 comments · Fixed by #308
Assignees
Labels
bug Something isn't working
Milestone

Comments

@mattrothenberg
Copy link
Contributor

Describe the bug

Howdy! I'm using @microsoft/fast-react-wrapper to convert these web-components into React components. I've noticed that, for some components, event handler props like onInput and onClick work totally fine, but for others, these props don't seem to do anything. For example, say I have the following wrapped React components 👇

import { provideReactWrapper } from '@microsoft/fast-react-wrapper';
import {
  vsCodeButton,
  vsCodeDropdown,
  vsCodeOption,
  vsCodeTextField,
} from '@vscode/webview-ui-toolkit';

const { wrap } = provideReactWrapper(React, provideFASTDesignSystem());

const VSCodeButton = wrap(vsCodeButton());
const VSCodeDropdown = wrap(vsCodeDropdown());
const VSCodeOption = wrap(vsCodeOption());
const VSCodeTextField = wrap(vsCodeTextField());

I'm able to use the onClick prop of the VSCodeButton component without trouble.

<VSCodeButton
  onClick={() => {
    console.log('works! ✅');
  }}
>
  Click Me
</VSCodeButton>

Likewise, the onInput prop of the VSCodeTextField component works just fine.

<VSCodeTextField
  onInput={(e) => {
    console.log('i see this logged ✅');
  }}
/>

However, the onChange prop of VSCodeDropdown never seems to fire.

<VSCodeDropdown
  onChange={(e) => {
    console.log('never works ❌');
  }}
>
  <VSCodeOption>Value #1</VSCodeOption>
  <VSCodeOption>Value #2</VSCodeOption>
  <VSCodeOption>Value #3</VSCodeOption>
</VSCodeDropdown>

Additionally, I've noticed that the prop types for the generated components aren't totally right. Here, appearance should be a valid prop, but TS throws an error.

Screen Shot 2021-11-03 at 10 16 29 AM

Note that this doesn't happen for the wrapped fastButton component.

import { fastButton, provideFASTDesignSystem } from '@microsoft/fast-components';

const { wrap } = provideReactWrapper(React, provideFASTDesignSystem());

const FastButton = wrap(fastButton());

Screen Shot 2021-11-03 at 10 17 54 AM

Expected behavior

I'd expect these props to work consistently across the generated React components, but it's quite likely that I'm misunderstand the respective APIs of these components.

Do you have any idea what might be going on?

Desktop (please complete the following information):

  • Toolkit Version: 0.8.3
@mattrothenberg mattrothenberg added the bug Something isn't working label Nov 3, 2021
@hawkticehurst
Copy link
Member

Thanks, @mattrothenberg!

Some initial quick thoughts/questions are if this lack of prop behavior is showing up in more than the dropdown? If it is only the dropdown, I wonder if this could be related to #181 and subsequently this issue on FAST. 🧐 (cc @chrisdholt for thoughts)

Also weird that the type information for the appearance attribute is not showing up either. I can dive into that more once I start up on the React sample extension work in a few days, but again going to cc @chrisdholt in case he has some immediate thoughts/insights into what might be happening.

@mattrothenberg
Copy link
Contributor Author

@hawkticehurst Good question. I admittedly haven't run through every component, but the lack of prop behavior seems to be sporadic. Part of the problem is that I don't know which props to expect, so it could ultimately just be an education/documentation sort of issue.

I'll keep on digging and add stuff to this issue as I find it

@mattrothenberg
Copy link
Contributor Author

mattrothenberg commented Nov 3, 2021

For instance it turns out that I just might be using the dropdown component incorrectly 🤦 . The following code works, though I still get a weird type error saying that value isn't a valid prop for the option component.

<VSCodeDropdown
  onInput={(e) => {
    console.log(e);
  }}
>
  <VSCodeOption>Value #1</VSCodeOption>
  <VSCodeOption>Value #2</VSCodeOption>
  <VSCodeOption>Value #3</VSCodeOption>
</VSCodeDropdown>

Screen Shot 2021-11-03 at 12 37 55 PM

@hawkticehurst
Copy link
Member

@mattrothenberg thought I'd give you a heads up that there is a conversation going on in another issue thread that might be very relevant/helpful to what we're talking about here.

#283 (comment)

@hawkticehurst
Copy link
Member

Also thanks for the updates! This is helpful information for me to keep in mind as I start to work on the React sample/docs.

@mattrothenberg
Copy link
Contributor Author

@hawkticehurst Of course! I'm totally down to 🍐 on the React docs with you if you need an extra hand/brain :)

@hawkticehurst
Copy link
Member

Ooh, I will happily take you up on that offer! Thank you!

I have some low-hanging issues/bugs that I'm trying to tackle this week, but I'll set up a time to chat sometime next week once I'm a bit more started up on the React work.

@xsteadybcgo
Copy link

For instance it turns out that I just might be using the dropdown component incorrectly 🤦 . The following code works, though I still get a weird type error saying that value isn't a valid prop for the option component.

<VSCodeDropdown
  onInput={(e) => {
    console.log(e);
  }}
>
  <VSCodeOption>Value #1</VSCodeOption>
  <VSCodeOption>Value #2</VSCodeOption>
  <VSCodeOption>Value #3</VSCodeOption>
</VSCodeDropdown>
Screen Shot 2021-11-03 at 12 37 55 PM

I also encountered this type error, I guess it's the type error of fast-react-wrapper package, maybe need to replace ReactModule.HTMLAttributes with ReactModule.AllHTMLAttributes , how should I define a type to solve it now?

@hawkticehurst
Copy link
Member

This issue should be fixed now!

The problem was the result of a limitation in how TypeScript does type inference––you can read more about it in this issue. The solution was to just be more explicit in adding the correct types when registering toolkit components.

Also as a heads up, I'm probably going to wait a bit to publish the changes so that the React component work also gets published at the same time.

@kevcao-certik
Copy link

@hawkticehurst It appears this type issue has appeared again:

          <VSCodeTextArea
            name="description"
            value={descContent}
            onInput={(e) => setDescContent(e.target?.value)}
            required
          />

image

I'm currently using @vscode/[email protected] and [email protected].

@hawkticehurst
Copy link
Member

Shoot well that's disappointing to hear. 😔 Thank you for the heads up, however!

As a heads up, we're pretty tightly resourced at this time so it might take a bit of time for me to dive into this but I'll try and put this closer to the top of my priority list during the next few weeks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants