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

Improve JSX definitions #3801

Merged
merged 6 commits into from
Jul 7, 2022
Merged

Improve JSX definitions #3801

merged 6 commits into from
Jul 7, 2022

Conversation

Princesseuh
Copy link
Member

@Princesseuh Princesseuh commented Jul 2, 2022

Changes

This does many things to our JSX definitions, all improvements, hopefully!

  • Fixed attributes needing numbers not accepting strings, especially ARIA attributes
  • Added element-specific attributes instead of just a list of all possible attributes, this allow for better type checking (adding an attribute not allowed on a specific element but allowed on others will now show an error)
  • Added some missing attributes

Testing

Types aren't tested at the moment. When astro check get upgraded to use the latest language-server, it would be possible to create an absolutely MASSIVE Astro file with ALL the HTML elements attributes (with their possible values) and run the checks on it to make sure there's no errors

Docs

N/A

@changeset-bot
Copy link

changeset-bot bot commented Jul 2, 2022

🦋 Changeset detected

Latest commit: bbe49e0

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 9 packages
Name Type
astro Patch
@e2e/astro-component Patch
@e2e/lit-component Patch
@e2e/preact-component Patch
@e2e/react-component Patch
@e2e/solid-component Patch
@e2e/svelte-component Patch
@e2e/e2e-tailwindcss Patch
@e2e/ts-resolution Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions github-actions bot added the pkg: astro Related to the core `astro` package (scope) label Jul 2, 2022
@carlcs
Copy link

carlcs commented Jul 2, 2022

Hi @Princesseuh, another attribute that is currently missing is inert. https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement/inert

I added it manually to my project, but would appreciate if it was there by default.

declare global {
  namespace astroHTML.JSX {
    interface HTMLAttributes {
      /**
       * The HTMLElement property inert is a boolean value that, when present, makes the browser "ignore" user input events for the element, including focus events
       * and events from assistive technologies. The browser may also ignore page search and text selection in the element. This can be useful when building UIs
       * such as modals where you would want to "trap" the focus inside the modal when it's visible.
       * @see https://github.com/WICG/inert
       */
      inert?: boolean | string | undefined | null
    }
  }
}

@Princesseuh
Copy link
Member Author

Hi @Princesseuh, another attribute that is currently missing is inert. https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement/inert

I added it manually to my project, but would appreciate if it was there by default.

declare global {
  namespace astroHTML.JSX {
    interface HTMLAttributes {
      /**
       * The HTMLElement property inert is a boolean value that, when present, makes the browser "ignore" user input events for the element, including focus events
       * and events from assistive technologies. The browser may also ignore page search and text selection in the element. This can be useful when building UIs
       * such as modals where you would want to "trap" the focus inside the modal when it's visible.
       * @see https://github.com/WICG/inert
       */
      inert?: boolean | string | undefined | null
    }
  }
}

Thank you very much, I'll add it!

@Princesseuh Princesseuh marked this pull request as ready for review July 6, 2022 14:35
@Princesseuh Princesseuh changed the title [WIP] Improve JSX definitions Improve JSX definitions Jul 6, 2022
oncopy?: ClipboardEventHandler<T> | string | undefined | null;
oncut?: ClipboardEventHandler<T> | string | undefined | null;
onpaste?: ClipboardEventHandler<T> | string | undefined | null;
oncopy?: string | undefined | null;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the idea here that an Astro component will always have a string here rather than the actual EventHandler<T> object inlined from frontmatter? Would this break XElement?

Copy link
Member Author

@Princesseuh Princesseuh Jul 6, 2022

Choose a reason for hiding this comment

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

I doubt it would break XElement, because it doesn't make you write HTML events

However, yes, the following now result in an error in the editor:

---
function hello() {
	alert("Hey")
}
---

<div onclick={hello}></div>

But then again, this code does not work in Astro either - the serialized function you get is wrong, so I'm not sure how to proceed.

Either way using TypeScript's DOM types feels weird in Astro, they're much more intended for when you're manipulating events through JavaScript and not when you're writing HTML. If we want to allow serialized functions, updating the type to onclick?: () => any | string | undefined | null; would probably do it, I think

@Princesseuh Princesseuh merged commit b84bd7d into main Jul 7, 2022
@Princesseuh Princesseuh deleted the jsx-definitions-improvements branch July 7, 2022 18:44
@astrobot-houston astrobot-houston mentioned this pull request Jul 7, 2022
SiriousHunter pushed a commit to SiriousHunter/astro that referenced this pull request Feb 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: astro Related to the core `astro` package (scope)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants