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

feat(qwik-city): add a scripts key to head #3182

Closed
wants to merge 4 commits into from

Conversation

kemilbeltre
Copy link

@kemilbeltre kemilbeltre commented Feb 27, 2023

Allow for injecting custom scripts

#2593

What is it?

  • Feature / enhancement
  • Bug
  • Docs / tests

Description

Added a scripts key to head to allow for injecting custom scripts.

Checklist:

  • My code follows the developer guidelines of this project
  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • Added new tests to cover the fix / functionality

Allow for injecting custom scripts

2593
@stackblitz
Copy link

stackblitz bot commented Feb 27, 2023

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

async?: string;
crossorigin?: string;
defer?: string;
fectpriority?: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

fetchpriority I'm guessing

@kemilbeltre kemilbeltre marked this pull request as ready for review March 1, 2023 09:23
@kemilbeltre kemilbeltre requested a review from ulic75 March 1, 2023 09:23
@Harkunwar
Copy link
Contributor

type?: string;
importmap?: string;
blocking?: string;
id?: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should there also be a property to have a script which might go inside the script tag, maybe script or innerHtml ?

Copy link
Contributor

Choose a reason for hiding this comment

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

what do you think about using the HTMLScriptElement typing?

Copy link
Author

@kemilbeltre kemilbeltre Mar 1, 2023

Choose a reason for hiding this comment

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

I think 🤔 there is not extra property for that. Please check the official specifications here.

Or do you meant like a custom property?

@zanettin zanettin added the TYPE: enhancement New feature or request label Mar 1, 2023
@kemilbeltre
Copy link
Author

kemilbeltre commented Mar 1, 2023

Should the starter and the docs files be updated too? Such as this one: https://github.com/BuilderIO/qwik/blob/7a0e9f1f02cf6bfb33658722e02e22789f6a6aee/packages/docs/src/components/router-head/router-head.tsx

Like this (?):

     {head.scripts.map((sc) => (
          <script {...sc}></script>
      ))}

@Harkunwar
Copy link
Contributor

Harkunwar commented Mar 1, 2023

Should the starter and the docs files be updated too? Such as this one: https://github.com/BuilderIO/qwik/blob/7a0e9f1f02cf6bfb33658722e02e22789f6a6aee/packages/docs/src/components/router-head/router-head.tsx

Like this (?):

     {head.scripts.map((sc) => (
          <script {...sc}></script>
      ))}

Something like the following maybe:

     {head.scripts.map(({ script, ...props }) => (
          <script {...props} dangerouslySetInnerHTML={script} />
      ))}

Similar to how it's down for the style tag, https://github.com/BuilderIO/qwik/blob/7a0e9f1f02cf6bfb33658722e02e22789f6a6aee/packages/docs/src/components/router-head/router-head.tsx#L46

@Harkunwar
Copy link
Contributor

I also worked on this before I saw your PR, maybe I can push my version later today as it had much more in depth changes.

@Harkunwar
Copy link
Contributor

Sorry for creating another PR. This is what I was proposing:
#3230

@kemilbeltre kemilbeltre closed this Mar 2, 2023
@kemilbeltre kemilbeltre deleted the feat-scripts-key-head branch March 2, 2023 19:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
TYPE: enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants