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

supports uri fragment in static style transformed require #37

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

UncleBill
Copy link

@@ -6,6 +6,8 @@ export interface Attr {
export interface ASTNode {
tag: string
attrs: Attr[]
staticStyle?: string
Copy link
Member

@znck znck Oct 28, 2018

Choose a reason for hiding this comment

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

I am not convinced in changing the interface of ASTNode, it's not necessary to surface staticStyle from attrs. Also now codegen has to modified to treat styles differently.

Copy link
Author

Choose a reason for hiding this comment

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

I am not convinced in changing the interface of ASTNode, it's not necessary to surface staticStyle from attrs.

since attrs doesn't contain style

Also now codegen has to modified to treat styles differently.

Agree

Copy link
Member

@znck znck left a comment

Choose a reason for hiding this comment

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

I would rather extend implementation of rewrite method to handle asset URLs in inline style.

If we transform, asset URLs in static style attribute then behaviour of bounded style attribute should be same, which can be little difficult to achieve.

<div style="background: url('/some/image/path/some-file.png')">

and

<div :style="`background: url('/some/image/path/${filename}.png')`">
<div :style="`background: ${backgroundLink}`">
<div :style="`${backgroundStyle}`">

would produce same inline style but only first can be statically transformed to require statement. It leaves us with very confused state.

@UncleBill
Copy link
Author

I would rather extend implementation of rewrite method to handle asset URLs in inline style.

In fact, Node.attrs doesn't contain style attribute, or should we modify the compiler?

image

If we transform, asset URLs in static style attribute then behaviour of bounded style attribute should be same, which can be little difficult to achieve.

We don't transform bounded style, we can require resources explicitly.

In these cases:

<div :style="`background: url('/some/image/path/${filename}.png')`">
<!-- developers rely on "/some/image/path/" -->
<div :style="`background: ${backgroundLink}`">
<!-- this.backgroundLink = require('path/to/image') -->
<div :style="`${backgroundStyle}`">
<!-- this.backgroundStyle = `background: url${require('path/to/image')}` -->

@znck
Copy link
Member

znck commented Oct 29, 2018

On compiled render function vnode data, staticStyle is intact an object.

see template explorer

@UncleBill
Copy link
Author

UncleBill commented Oct 30, 2018

Yes, staticStyle will be an object in compiled render function.

I tested in vue-loader's example, it works just fine.

Here is my test steps:

  1. Hard code default transformOption, add { '*': 'style'} Add following options to vue-loader in webpack.config.js
        options: {
          transformAssetUrls: {
            '*': 'style'
          }
        }
  1. Goto vue-loader directory, execute npm install path/to/component-utils
  2. Add style with background image to the example page
<template lang="pug">
div(ok)
  h1(:class="$style.red") hello
  h1(style="background:url(./image/foo.png)") hello
</template>
  1. Config file-loader in example/webpack.config.js
  2. npm run dev or npm run build

Then the compiled code is

var render = function() {
  var _vm = this
  var _h = _vm.$createElement
  var _c = _vm._self._c || _h
  return _c("div", { attrs: { ok: "" } }, [
    _c("h1", { class: _vm.$style.red }, [_vm._v("hello")]),
    _c(
      "h1",
      {
        staticStyle: { background: "url(" + __webpack_require__(/*! ../image/foo.png */ "./image/foo.png") + ")" }
      },
      [_vm._v("hello")]
    )
  ])
}
var staticRenderFns = []
render._withStripped = true

Run on browser:
image

@znck
Copy link
Member

znck commented Oct 30, 2018

/cc @yyx990803 I need your opinion on this.

@UncleBill
Copy link
Author

@yyx990803

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

background-image in style property, what is the best practice?
2 participants