-
Notifications
You must be signed in to change notification settings - Fork 192
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
fix(base,reactive-controllers): add delegates API and downstate delegate #4229
base: main
Are you sure you want to change the base?
Conversation
… into jnjosh/theming-systems
* fix(slider): no change in background for variant filled and fill start * chore(slider): updated readme for slider with fill start and variant filled --------- Co-authored-by: Rajdeep Chandra <rajdeepchandra@Rajdeeps-MacBook-Pro-2.local>
Bumps [next](https://github.com/vercel/next.js) from 14.1.0 to 14.1.4. - [Release notes](https://github.com/vercel/next.js/releases) - [Changelog](https://github.com/vercel/next.js/blob/canary/release.js) - [Commits](vercel/next.js@v14.1.0...v14.1.4) --- updated-dependencies: - dependency-name: next dependency-type: direct:development update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…ctrum-web-components into jnjosh/theming-systems
ca038ec
to
12230c7
Compare
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.
Not so sure why there's churn in the lock. I've seen it in other branches lately, so it might be related to renovate bot, or this might not be fully up to date with main
?
); | ||
await nextFrame(); | ||
await nextFrame(); | ||
controller.handlePointerDown(); |
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 should use sendMouse
to trigger this, rather than address the method directly.
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.
Sure! the directly call is removed with a sendMouse trigger to mimic the call!
import { html, LitElement } from 'lit'; | ||
import { DownState } from '@spectrum-web-components/reactive-controllers/src/downstate.js'; | ||
|
||
class TestDownstateEl extends LitElement {} |
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.
Can we use spectrumConfig
here to ensure both the Controller and the delegates approach?
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.
May also want to wrap it all in an <sp-theme>
to get ALL of the contracts covered.
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.
class TestDownstateEl extends SpectrumElement {
override spectrumConfig = {
downstate: ['spectrum-two'],
};
protected override render(): TemplateResult {
return html`
<sp-theme system="spectrum-two" color="light" scale="medium">
<slot></slot>
</sp-theme>
`;
}
}
this.host?.removeEventListener('pointerdown', this.handlePointerDown); | ||
this.host?.removeEventListener('pointerup', this.handlePointerUp); |
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 should remove the listeners with the abort signal.
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.
removeEventListeners(): void {
this.abortController.abort();
this.host?.removeEventListener('pointerdown', this.handlePointerDown);
this.host?.removeEventListener('pointerup', this.handlePointerUp);
}
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.
The use of the abort controller here should prevent the need for removeEventListener
code.
public handlePointerDown = (): void => { | ||
this.updateDownStateStyles(true); | ||
}; | ||
|
||
public handlePointerUp = (): void => { | ||
this.updateDownStateStyles(false); | ||
}; |
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.
This applies these changes synchronously, which is probably correct. However, the majority of our render reactions are async, due to the reactivity of LitElement. Do you se any issue with this?
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 feel updating the down state styles is a UI-specific operation and not directly related to LitElement's reactivity, handling it synchronously shouldn't cause any problems, but would love to hear your thoughts why you foresee some issues here?
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.
Less about issues than about normalization and expectations. It's probably of no matter other than how it should be explicitly tested in https://github.com/adobe/spectrum-web-components/pull/4229/files/e9ea73081b0593fd29df23b1c67b00de68700cfb#r1589155295
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.
This has been updated!
…-components into delegate-controllers
6ca339b
to
33c5c24
Compare
Description
This PR adds the delegate strategy in SWC that allows us to add
downstate
functionality over our existing components. This is needed so that we can support features likedownstate
in Spectrum 2 while keeping everything as it is in Spectrum 1Downstate Activation is added to this PR
Motivation and context
The downstate feature in spectrum-two enhances user interaction by providing visual feedback when components are clicked or activated. It improves accessibility and user experience by clearly indicating the active state of elements. Developers can enable downstate for components, and it seamlessly integrates with existing functionality in the framework. This addition enriches the user interface, making interactions more intuitive and engaging.
Technical Document
How has this been tested?
Screenshots (if appropriate)
Types of changes
Checklist
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
.