-
Notifications
You must be signed in to change notification settings - Fork 88
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
[WNMGDS-2521] Add custom event handling to web components #2738
Conversation
…til solution can be found.
packages/design-system/src/components/Alert/useAlertAnalytics.ts
Outdated
Show resolved
Hide resolved
packages/design-system/src/components/web-components/ds-alert.test.tsx
Outdated
Show resolved
Hide resolved
// events.forEach((name) => { | ||
// Object.defineProperty(CustomElement.prototype, name, { | ||
// set(v) { | ||
// if (this.__mounted) { | ||
// this.attributeChangedCallback(name, null, v); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just had an idea that we could explore later: What if instead of trying to define the callback that they add as one of the props (which clobbers any listeners we made before) we instead added an event listener in this function? Like bob.onChange = myCallback
results in bob.addEventListener('ds-change', myCallback)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could pull the kebabCaseIt(name.replace('on', 'ds'))
logic out into its own function and then use it here in this set
function and in the proxyEvents
function
packages/design-system/src/components/web-components/preactement/define.ts
Show resolved
Hide resolved
packages/design-system/src/components/web-components/preactement/define.ts
Show resolved
Hide resolved
packages/design-system/src/components/web-components/preactement/define.ts
Outdated
Show resolved
Hide resolved
Since this is mostly about adding the event-handling code, could the PR title reflect that? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good work! This is an exciting feature
aria-labelledby="static-id__a11y-label" | ||
aria-labelledby="alert--1__a11y-label" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you know why this changed? I don't see code that actually defines the root-id
, but I also don't see that it was removed in this branch. Like I get why the static id isn't present, but why was it ever static before?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i noticed the static id wasn't set and figured this was a fix but no clue why it changed.
WNMGDS-2521