Skip to content
This repository was archived by the owner on Apr 6, 2023. It is now read-only.
/frameworkPublic archive

feat(nuxt): add fetchpriority attribute and literal typings for meta components - #6251

Merged
pi0 merged 18 commits into
nuxt:mainfrom
sanjaiyan-dev:sanjaiyan-fetchpiritory-attr
Aug 2, 2022
Merged

feat(nuxt): add fetchpriority attribute and literal typings for meta components#6251
pi0 merged 18 commits into
nuxt:mainfrom
sanjaiyan-dev:sanjaiyan-fetchpiritory-attr

Conversation

@sanjaiyan-dev

Copy link
Copy Markdown
Contributor

Added fetchpriority attr in script and link component.

@netlify

netlifyBot commented Jul 29, 2022

Copy link
Copy Markdown

Deploy Preview for nuxt3-docs canceled.

NameLink
🔨 Latest commit9ca4154
🔍 Latest deploy loghttps://app.netlify.com/sites/nuxt3-docs/deploys/62e91246fca631000893a819

@sanjaiyan-devsanjaiyan-dev changed the title Sanjaiyan-fetchpiritory-attrAdding fetchpriority attrJul 29, 2022
@danielroedanielroe changed the title Adding fetchpriority attrfeat(nuxt): add fetchpriority attr to <Script> and <Link>Jul 29, 2022
@DamianGlowala

Copy link
Copy Markdown
Member

Shall we narrow down the accepted values for the fetchpriority attribute, as per https://wicg.github.io/priority-hints/#definitions?

Comment threadpackages/nuxt/src/head/runtime/components.ts Outdated
Comment threadpackages/nuxt/src/head/runtime/components.ts Outdated
@sanjaiyan-dev

Copy link
Copy Markdown
ContributorAuthor

Shall we narrow down the accepted values for the fetchpriority attribute, as per https://wicg.github.io/priority-hints/#definitions?

💪🏽

@sanjaiyan-dev

Copy link
Copy Markdown
ContributorAuthor

Sorry, I have copied the code from #6262 for adding typings for reference-policy attribute 🙇🏽

@sanjaiyan-devsanjaiyan-dev changed the title feat(nuxt): add fetchpriority attr to <Script> and <Link>feat(nuxt): add fetchpriority attr to <Script> and <Link> and minor improvements in types :)Jul 30, 2022
@sanjaiyan-devsanjaiyan-dev changed the title feat(nuxt): add fetchpriority attr to <Script> and <Link> and minor improvements in types :)feat(nuxt): Add fetchpriority attr to <Script> and <Link> and minor improvements in types :)Jul 30, 2022
@DamianGlowala

Copy link
Copy Markdown
Member

Great enhancement! I have closed #6262 in favor of your changes. There are two remarks that I've got:

@sanjaiyan-dev

sanjaiyan-dev commented Jul 31, 2022

Copy link
Copy Markdown
ContributorAuthor

Great enhancement! I have closed #6262 in favor of your changes. There are two remarks that I've got:

  1. Boolean type is equivalent to crossorigin="" which is shorthand way of writing crossorigin="anonymous" so this makes typing code easier ,
<imgcrossorigin/>

is easier than

<imgcrossorigin="" />

And even browser parse the html with crossorigin attr empty string as <link crossorigin /> (not sure)
I am not 100% sure about it :
so, do we need to add boolean type too ?

@sanjaiyan-dev

Copy link
Copy Markdown
ContributorAuthor

Great enhancement! I have closed #6262 in favor of your changes. There are two remarks that I've got:

  1. Boolean type is equivalent to crossorigin="" which is shorthand way of writing crossorigin="anonymous" so this makes typing code easier ,
<img crossorigin />

is easier than

<img />

And even browser parse the html with crossorigin attr empty string as <link crossorigin /> (not sure) I am not 100% sure about it : so, do we need to add boolean type too ?

I am sorry if I am wrong :(

@DamianGlowala

Copy link
Copy Markdown
Member

All I am thinking is whether the following would be preferable:

crossorigin: String as PropType<CrossOrigin>

Let's wait for others to express their thoughts, not sure which way to go :)

@sanjaiyan-dev

Copy link
Copy Markdown
ContributorAuthor

All I am thinking is whether the following would be preferable:

crossorigin: String as PropType<CrossOrigin>

Let's wait for others to express their thoughts, not sure which way to go :)

Ok 🙌

@sanjaiyan-dev

Copy link
Copy Markdown
ContributorAuthor
typeLinkRelationship=|"alternate"|"author"|"canonical"|"dns-prefetch"|"help"|"icon"|"license"|"manifest"|"me"|"modulepreload"|"next"|"pingback"|"preconnect"|"prefetch"|"preload"|"prerender"|"prev"|"search"|"stylesheet"|String;

Added types for <link /> tag with String constructor to accept any future value while preserving intellisense 😄

Comment threadpackages/nuxt/src/head/runtime/components.ts Outdated

@danielroedanielroe left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Love the direction of this PR. ❤️ Would you revert the various style/lint changes that have been made by your IDE?

@sanjaiyan-dev

Copy link
Copy Markdown
ContributorAuthor

Love the direction of this PR. ❤️ Would you revert the various style/lint changes that have been made by your IDE?

Yeah sure 🙌

@sanjaiyan-dev

Copy link
Copy Markdown
ContributorAuthor

Made suggested change (not sure) 😄

@pi0
pi0 requested a review from danielroeAugust 2, 2022 11:44
@pi0pi0 changed the title feat(nuxt): Add fetchpriority attr to <Script> and <Link> and minor improvements in types :)feat(nuxt): add fetchpriority attribute to the <Script> and <Link> componentsAug 2, 2022
Comment threadpackages/nuxt/src/head/runtime/types.ts Outdated

@danielroedanielroe left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Really nice enhancement - thanks! ❤️

@danielroedanielroe changed the title feat(nuxt): add fetchpriority attribute to the <Script> and <Link> componentsfeat(nuxt): add fetchpriority attribute and literal typings for meta componentsAug 2, 2022
pi0
pi0 approved these changes Aug 2, 2022

@pi0pi0 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks!

@pi0
pi0 merged commit 94d0c08 into nuxt:mainAug 2, 2022
@sanjaiyan-dev

Copy link
Copy Markdown
ContributorAuthor

Thanks!

My pleasure 💪🙌

Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@sanjaiyan-dev@DamianGlowala@pi0@danielroe