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

Add Invoker Commands API #2411

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

@github-actions github-actions bot added the feature definition Creating or defining new features or groups of features. label Dec 9, 2024
Copy link
Collaborator

@jamesnw jamesnw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for submitting! I think this will be blocked until the spec is merged (or we are able to more comprehensively account for future features.

@@ -0,0 +1,12 @@
name: Invoker Commands
description: Adding commandfor and command attributes to elements would allow to assign interactions in a more accessible and declarative way, while reducing bugs and simplifying the amount of JavaScript pages are required to ship for interactivity.
spec: https://open-ui.org/components/invokers.explainer/
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this PR will be blocked until the spec is merged in whatwg/html#9841, and then we can link to that.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So no explainers or drafts as a spec link?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's OK to link to other things if there's no better link, but you have to create an exception and document when we ought to remove the exception:

const defaultAllowlist: allowlistItem[] = [
// [
// "https://example.com/spec/",
// "Allowed because…. Remove this exception when https://example.com/org/repo/pull/1234 merges."
// ]

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ddbeck should this be something we discuss as a potential future improvement? As we head into a new phase of the project where we focus more on bleeding-edge features, we'll very frequently run into this case where features only have early explainers, and no specs.
I have a feeling that adding to the specs.ts file everytime might not scale then.
Maybe open a new discussion issue?

Copy link
Collaborator

@ddbeck ddbeck Dec 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think you're right that it won't be scalable. I'd welcome an issue on this. I think it's part of a general question along the lines of, "what's the ideal workflow for features before they ship and standardize?"

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #2433

features/invoker-commands.yml Outdated Show resolved Hide resolved
@jamesnw jamesnw added the blocked Blocked waiting for something external label Dec 10, 2024
Co-authored-by: James Stuckey Weber <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked Blocked waiting for something external feature definition Creating or defining new features or groups of features.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants