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

Meta tag with image url content #618

Closed
roadmanfong opened this issue Sep 10, 2016 · 35 comments
Closed

Meta tag with image url content #618

roadmanfong opened this issue Sep 10, 2016 · 35 comments
Milestone

Comments

@roadmanfong
Copy link

I 'd like to add something like

    <meta property="og:image" content="./src/images/cover.png" />
    <meta name="twitter:image" content="./src/images/cover.png" />

I notice that it didn't handle image inside the meta tag.
And I found a solution here
webpack-contrib/html-loader#17 (comment)

    <meta property="og:image" content="${require(`./src/images/cover.png`)}" />
    <meta name="twitter:image" content="${require(`./src/images/cover.png`)}" />

All we have to do is minor change in html loader setting
loader: 'html?interpolate=require',

roadmanfong@48bb6cf

@gaearon gaearon added this to the 0.5.0 milestone Sep 10, 2016
@gaearon
Copy link
Contributor

gaearon commented Sep 10, 2016

Thanks for the suggestion.
I’m on the fence about the interpolate syntax but I’d like to keep this open and figure out a cohesive solution for this and related problems in a future release, so reopening.

@gaearon gaearon reopened this Sep 10, 2016
@kahneraja
Copy link

i'm super keen on resolving this. anyone got some tips or direction i can take here?

@kahneraja
Copy link

kahneraja commented Sep 17, 2016

I've noticed the following happens.

on build <link rel="shortcut icon" href="./src/favicon.ico"> becomes ><link rel="shortcut icon" href="/static/media/favicon.f9ba6907.ico">.

which is perfect. I want to extend that to other meta tags like <meta name="twitter:image:src" content="./src/title.jpg">

if anyone can point in the direction to PR that i'll do it :)

@gaearon
Copy link
Contributor

gaearon commented Sep 17, 2016

The way I want it to work is that any attribute value starting with ./ should be processed via file-loader. But this needs to be implemented as a feature in html-loader and I’m not sure if they’re interested.

@kahneraja
Copy link

How does <link rel="shortcut icon" href="./src/favicon.ico"> pick up required build change?

@gaearon - Is that something covered off in file-loader?

@gaearon
Copy link
Contributor

gaearon commented Sep 17, 2016

No, it’s part of html-loader (usage).

We currently specify link:href as attrs option so that’s the only thing it picks up.
I want it to pick up any attribute value starting with ./: webpack-contrib/html-loader#17 (comment).

Relevant code is pretty small: https://github.com/webpack/html-loader/blob/master/index.js

@kahneraja
Copy link

thanks @gaearon i'll look into it :)

@kahneraja
Copy link

ah. i know understand we are looking at how this config gets picked up...
https://github.com/facebookincubator/create-react-app/blob/master/config/webpack.config.prod.js#L173

@gaearon
Copy link
Contributor

gaearon commented Sep 17, 2016

Yep, that’s where it is.

@kahneraja
Copy link

do we want to take a href-content place holder attribute approach?

<meta name="twitter:image:src" href-content="./src/title.jpg">
becomes
<meta name="twitter:image:src" content="http://wetrumphate.com/static/media/title.3c0fcf5e.jpg">

@kahneraja
Copy link

hmm. i guess the idea of content starting with "./" is sort of better.

@gaearon
Copy link
Contributor

gaearon commented Sep 17, 2016

I can’t really imagine why somebody would want to start their attributes with ./ and not mean a local file, so I think it will be safe to just assume that’s the intention.

@kahneraja
Copy link

additional notes

Pre configured:
["img:src"]

React Scripts configuration:
["link:href"]

New potential tags:
["meta:content"] // if content starts with "./"

@kahneraja
Copy link

hmm i'm guessing the html-loader guys are not going to approve cause they already have advice on this.
https://github.com/webpack/html-loader#interpolation

@kahneraja
Copy link

you think it's worth pushing for a change?

@kahneraja
Copy link

we could do both.

change the prod config in thiscreate-react-app and also push for that change in html-loader

what's your take @gaearon ?

@gaearon
Copy link
Contributor

gaearon commented Sep 17, 2016

hmm i'm guessing the html-loader folks are not going to approve cause they already have advice on this

I don’t see why not. Interpolation is a possible solution but I haven’t seen ./ being considered before. I think it could work fine as an option (even if not the default).

change the prod config in thiscreate-react-app and also push for that change in html-loader

If we just add meta:content, wouldn’t it break if your content is something other than a file? e.g. <meta name="description" content="my website"> If not maybe we can just do that.

@kahneraja
Copy link

i guess the minimum we could do is just push this...
roadmanfong@48bb6cf

and then encourage people to do the approach @roadmanfong mentioned.
<meta property="og:image" content="${require(./src/images/cover.png)}" />

shall i set up a PR?

@gaearon
Copy link
Contributor

gaearon commented Sep 18, 2016

I’m not really happy supporting this long term and adding custom syntax to HTML.

@kahneraja
Copy link

Ok no problem. Ill just do that as a temp work around and try to push to the html-loader proj later.

@gaearon
Copy link
Contributor

gaearon commented Sep 18, 2016

👍

@gaearon
Copy link
Contributor

gaearon commented Sep 18, 2016

Meh. I think let’s go for ${} syntax since it’s explicit and gives user the control.

@kahneraja

Would you mind submitting a PR that removes automatic detection of link:href but adds ${} syntax?

@kahneraja
Copy link

kahneraja commented Sep 19, 2016

@gaearon can i just confirm you mean to add 2 separate PRs.

one to make html-loader pick up on any meta tag with content like this?

<meta name="twitter:image:src" content="${`./src/img/title.jpg`}">

and one to remove that config settings from create-react-app?

... attrs: ['link:href'], // remove

@kahneraja
Copy link

kahneraja commented Sep 19, 2016

@gaearon another thing.

from looking at this test it does sort of look like the html-loader guys are keen on sticking with the $(require('...')) approach.
https://github.com/webpack/html-loader/blob/master/test/loaderTest.js#L122

@zhuqiacheng? @sheerun?

@kahneraja
Copy link

also... is there much of a difference between ${./src/img/title.jpg} and $require{./src/img/title.jpg}?

in my head they seem same-ish... ?

@sheerun
Copy link
Contributor

sheerun commented Sep 19, 2016

As React shown, javascript is better than any templating language. Allowing ${} interpolation inside html allows not only with full webpack integration (like src="${require('../image.png')}", but also allows for seamless templating <div id="header">${require("../partials/header.html")}</div>. It's the easiest way to integrate with webpack dependency traversal.

For me allowing interpolation is both more powerful and actually easier to maintain in long run.

@kahneraja
Copy link

If i understand things correctly, @gaearon i do tend to agree with @sheerun.

the standard interpolate approach is flexible and possibly easier to maintain.

If that's the case then fix to this would a PR to the 'create-react-app' to line up config and html files.

@gaearon
Copy link
Contributor

gaearon commented Sep 19, 2016

OK, will accept a PR that adds interpolation syntax but disables automatic handling of link:href.

@kahneraja
Copy link

on it :)

@kahneraja
Copy link

getting a serious of failures when attempting to require interpolation with the dev config.
image
investigating....

@gaearon
Copy link
Contributor

gaearon commented Sep 19, 2016

Maybe there’s some kind of typo in index.html?

@gaearon
Copy link
Contributor

gaearon commented Sep 22, 2016

Meh, interpolation syntax is super annoying and hard to understand.
I’m working on another solution to this, stay tuned!

@gaearon
Copy link
Contributor

gaearon commented Sep 22, 2016

I drafted a proposal to solve this in #703. Unless we find some fatal flaws, it should come out in 0.5.0. Let me know what you think!

@gaearon
Copy link
Contributor

gaearon commented Sep 22, 2016

Closing as this is fixed, and will be released in 0.5.0.

@gaearon gaearon closed this as completed Sep 22, 2016
@gaearon
Copy link
Contributor

gaearon commented Sep 23, 2016

This is now supported in 0.5.0.

Read about using the new public folder.

See also migration instructions and breaking changes in 0.5.0.

@lock lock bot locked and limited conversation to collaborators Jan 22, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants