chore(checkbox): declare change event in customer-elements.json #3061

jianliao · 2023-03-27T05:04:07Z

Description

It looks like the customer-elements.json for Checkbox component hasn't mentioned its change event.

Related issue(s)

N/A

Motivation and context

There is no change event declaration in customer-elements.json which cause @swc-react/checkbox component is unable to support change event callback.

How has this been tested?

Test case 1
1. Go here
2. Do this
Test case 2
1. Go here
2. Do this

Screenshots (if appropriate)

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Chore (minor updates related to the tooling or maintenance of the repository, does not impact compiled assets)

Checklist

I have signed the Adobe Open Source CLA.
My code follows the code style of this project.
If my change required a change to the documentation, I have updated the documentation in this pull request.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
All new and existing tests passed.
I have reviewed at the Accessibility Practices for this feature, see: Aria Practices

Best practices

This repository uses conventional commit syntax for each commit message; note that the GitHub UI does not use this by default so be cautious when accepting suggested changes. Avoid the "Update branch" button on the pull request and opt instead for rebasing your branch against main.

github-actions · 2023-03-27T05:07:08Z

Branch Preview

Visual regression test results

When a visual regression test fails (or has previously failed while working on this branch), its results can be found in the following URLs:

github-actions · 2023-03-27T05:16:22Z

Tachometer results

Chrome

card permalink

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	438 kB	109.22ms - 112.19ms	-	faster ✔ 1% - 5% 1.18ms - 6.12ms
branch	438 kB	112.38ms - 116.33ms	slower ❌ 1% - 6% 1.18ms - 6.12ms	-

checkbox permalink

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	338 kB	92.46ms - 97.88ms	-	unsure 🔍 -4% - +2% -3.98ms - +2.24ms
branch	339 kB	94.51ms - 97.57ms	unsure 🔍 -2% - +4% -2.24ms - +3.98ms	-

split-view permalink

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	309 kB	62.67ms - 65.91ms	-	unsure 🔍 -4% - +2% -2.56ms - +1.44ms
branch	310 kB	63.69ms - 66.03ms	unsure 🔍 -2% - +4% -1.44ms - +2.56ms	-

switch permalink

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	303 kB	47.88ms - 50.12ms	-	unsure 🔍 -1% - +5% -0.58ms - +2.47ms
branch	304 kB	47.02ms - 49.09ms	unsure 🔍 -5% - +1% -2.47ms - +0.58ms	-

table permalink

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	430 kB	647.25ms - 658.80ms	-	unsure 🔍 -1% - +2% -8.38ms - +11.86ms
branch	431 kB	642.97ms - 659.60ms	unsure 🔍 -2% - +1% -11.86ms - +8.38ms	-

Firefox

card permalink

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	438 kB	181.84ms - 188.96ms	-	unsure 🔍 -5% - +1% -9.67ms - +2.19ms
branch	438 kB	184.40ms - 193.88ms	unsure 🔍 -1% - +5% -2.19ms - +9.67ms	-

checkbox permalink

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	338 kB	209.55ms - 217.05ms	-	unsure 🔍 -3% - +2% -6.70ms - +4.90ms
branch	339 kB	209.78ms - 218.62ms	unsure 🔍 -2% - +3% -4.90ms - +6.70ms	-

split-view permalink

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	309 kB	135.03ms - 144.09ms	-	unsure 🔍 -6% - +5% -8.33ms - +7.01ms
branch	310 kB	134.03ms - 146.41ms	unsure 🔍 -5% - +6% -7.01ms - +8.33ms	-

switch permalink

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	303 kB	110.10ms - 117.90ms	-	unsure 🔍 -7% - +3% -8.32ms - +3.44ms
branch	304 kB	112.04ms - 120.84ms	unsure 🔍 -3% - +7% -3.44ms - +8.32ms	-

table permalink

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	430 kB	1159.62ms - 1168.70ms	-	unsure 🔍 -1% - +0% -13.20ms - +0.04ms
branch	431 kB	1165.92ms - 1175.56ms	unsure 🔍 -0% - +1% -0.04ms - +13.20ms	-

Westbrook

Looks good to me, but could you help me understand the underlying situation a bit better here?

Specifically, change is a native event, does React really not allow users to bing onChange to any element? Does it have something to do with <SpCheckbox> not actually being the <sp-checkbox> element so it gets confused from an event bubbling perspective? Or maybe because we dispatch a Custom Event called change?

As we develop a better root understanding of the relationship between our HTML elements and the React components that wrap them we can do a better job of developing, reviewing, or tooling the code to ensure things like this are included system wide and not just each time we run into something that you or another client specifically needs in the moment.

jianliao · 2023-03-28T03:54:31Z

According to the React doc(old and new), onChange synthetic event is only supported on HTML form field native(browser built-in) element like input, select and textarea. Perhaps, it is because sp-checkbox is a customer element that wrapped an input element. The machine generated react wrapper is not a physical html element that wrapped the SWC customer element. It is like a development time placeholder.

jianliao · 2023-03-28T05:05:36Z

Hi @Westbrook, I'm afraid I'm confusing the typescript type issue between swc react wrapper and my own nextjs wrapper. React Synthetic event should be able to apply to all the HTMLElment type react components. Closed the issue.

jianliao · 2023-03-28T21:43:11Z

Hi @Westbrook,
I found this comment from React#19846, React would not dispatch onChange synthetic event for change native event to none native browser component. The following react code would compile, but onChange call back handler would never be called.

const handleChange = (e: FormEvent) => {
  console.log((e.currentTarget as CheckboxType).checked);
};
return (
    <Theme theme="spectrum" scale="medium" color="light">
      <div className="App">
        <Checkbox emphasized onChange={handleChange}>It is a SWC checkbox</Checkbox>
      </div>
    </Theme>
  )

For react wrapper code generation to work, we have to explicit declare it as an event.

I also audited the whole code base and found another missing declaration in [SplitView] (

spectrum-web-components/packages/split-view/src/SplitView.ts

Lines 403 to 409 in d4e49f5

    
           private dispatchChangeEvent(): void { 
        
               const changeEvent = new Event('change', { 
        
                   bubbles: true, 
        
                   composed: true, 
        
               }); 
        
               this.dispatchEvent(changeEvent); 
        
           }

) component. Let me know if you want me to add the event declaration.

Westbrook

Got it! LGTM. 🙇🏼

Next release may not be till next week, but you'll see it in the channel when it comes.

jianliao requested a review from Westbrook March 27, 2023 05:04

Westbrook previously approved these changes Mar 28, 2023

View reviewed changes

jianliao closed this Mar 28, 2023

jianliao reopened this Mar 28, 2023

jianliao dismissed Westbrook’s stale review via 44bd0ef March 29, 2023 15:50

jianliao force-pushed the checkbox-change-event branch from 44bd0ef to b16a749 Compare March 29, 2023 16:04

jianliao added 2 commits March 29, 2023 15:13

chore: declare change event in customer-elements.json

d7486d3

chore: declare change event in customer-elements.json for split view

63018fb

Westbrook force-pushed the checkbox-change-event branch from b16a749 to 63018fb Compare March 29, 2023 19:13

Westbrook approved these changes Mar 29, 2023

View reviewed changes

Westbrook merged commit fb5e536 into main Mar 29, 2023

Westbrook deleted the checkbox-change-event branch March 29, 2023 20:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(checkbox): declare change event in customer-elements.json #3061

chore(checkbox): declare change event in customer-elements.json #3061

jianliao commented Mar 27, 2023 •

edited

Loading

github-actions bot commented Mar 27, 2023

github-actions bot commented Mar 27, 2023 •

edited

Loading

card permalink

checkbox permalink

split-view permalink

switch permalink

table permalink

card permalink

checkbox permalink

split-view permalink

switch permalink

table permalink

Westbrook left a comment

jianliao commented Mar 28, 2023 •

edited

Loading

jianliao commented Mar 28, 2023

jianliao commented Mar 28, 2023

Westbrook left a comment

chore(checkbox): declare change event in customer-elements.json #3061

chore(checkbox): declare change event in customer-elements.json #3061

Conversation

jianliao commented Mar 27, 2023 • edited Loading

Description

Related issue(s)

Motivation and context

How has this been tested?

Screenshots (if appropriate)

Types of changes

Checklist

Best practices

github-actions bot commented Mar 27, 2023

Branch Preview

github-actions bot commented Mar 27, 2023 • edited Loading

Tachometer results

card permalink

checkbox permalink

split-view permalink

switch permalink

table permalink

card permalink

checkbox permalink

split-view permalink

switch permalink

table permalink

Westbrook left a comment

Choose a reason for hiding this comment

jianliao commented Mar 28, 2023 • edited Loading

jianliao commented Mar 28, 2023

jianliao commented Mar 28, 2023

Westbrook left a comment

Choose a reason for hiding this comment

jianliao commented Mar 27, 2023 •

edited

Loading

github-actions bot commented Mar 27, 2023 •

edited

Loading

jianliao commented Mar 28, 2023 •

edited

Loading