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

Allow adding media attribute to picture element #48

Open
1 of 3 tasks
chrispreisler opened this issue Mar 28, 2024 · 13 comments
Open
1 of 3 tasks

Allow adding media attribute to picture element #48

chrispreisler opened this issue Mar 28, 2024 · 13 comments
Labels
enhancement New feature or request

Comments

@chrispreisler
Copy link

chrispreisler commented Mar 28, 2024

Describe the feature

Hey,

I love your plugin. It is super helpful. I now have the case where I would like to add media attributes to a picture element with your plugin, but unfortunately it is not possible right now, correct? When I add media property to the sources array, it doesn't show up in the code.

So I would like to get something like this:

  <picture>
    <source
      media="(orientation: landscape)"
      srcset="image-320w.jpg 320w, image-640w.jpg 640w"
    />
    <source
      media="(orientation: portrait)"
      srcset="image-320w.jpg 320w, image-640w.jpg 640w"
    />
    <img src="/images/foo.jpg" />
  </picture>

Is it possible to allow this for the sourcesObject:

const exampleSources = [
  {
    media: "(orientation: landscape)",
    srcSet: "image-320w.jpg 320w, image-640w.jpg 640w",
  },
  {
    media: "(orientation: portrait)",
    srcSet: "image-320w.jpg 320w, image-640w.jpg 640w",
  },
];

Thanks for your help in advance.

Best regards
Chris

Additional information

  • Would you be willing to help implement this feature?
  • Can you think of other implementations of this feature?

Final checks

@chrispreisler
Copy link
Author

Hey, just a short heads up. Would it be possible to integrate this or don't you plan it, right now? Thanks a lot for your feedback on this feature :)

I use the Nuxt Module. Isn't it there quite easy to add it to the sources object and then add it to the picture element, quite similiar to the type property.

UnLazyImage.vue:

    sources?: {
      type: string
      srcSet: string
      sizes?: string
      media?: string
    }[]
    <source
      v-for="(source, index) in props.sources"
      :key="index"
      :type="source.type"
      :media="source.media"
      :data-srcset="source.srcSet"
      :data-sizes="source.sizes"
    >

@johannschopplich
Copy link
Owner

Hi there!
Thanks for the enhancement idea. While I don't have a roadmap for unlazy, media support for source sets is definitely on the list. It should be fairly easy to implement.

Unfortunately, nor do I have the usecase, neither the time to implement it at the moment. PR welcome! 🙌

@johannschopplich johannschopplich added the enhancement New feature or request label Apr 15, 2024
@chrispreisler
Copy link
Author

Hey Johann,

thanks for the feedback. I wanted to create a pull request, but when I fork and clone the project and start it locally, I get this error all the time: "Pre-transform error: Failed to resolve import "unlazy" from "src/runtime/components/UnLazyImage.vue". Does the file exist?"

It seems, that it can't resolve the aliases in vite.resolve.path {}. I also tried starting the vue moduel and the nuxt module, but in both I get the same error. Do you know why this is happening?

Best regards
Chris

@johannschopplich
Copy link
Owner

Right, there is currently a bug in unbuild with dev mode. That's why you need to run pnpm build once at root level or in packages/unlazy to build the core package first, so that the Nuxt module can use the unlazy package.

@chrispreisler
Copy link
Author

Ah okay thanks for the help. Unfortunately I can't build the unlazy package. Any idea why this is happenning? Sorry for bothering you.

RollupError: Could not resolve entry module "../../../../Local%20Development/unlazy/packages/unlazy/src/bundle/index.iife.ts".
│ at getRollupError (file:///Users/chrispreisler/Library/CloudStorage/Dropbox/Local%20Development/unlazy/node_modules/.pnpm/rollup@4.14.3/node_modules/rol…
│ at error (file:///Users/chrispreisler/Library/CloudStorage/Dropbox/Local%20Development/unlazy/node_modules/.pnpm/rollup@4.14.3/node_modules/rollup/dist/…
│ at ModuleLoader.loadEntryModule (file:///Users/chrispreisler/Library/CloudStorage/Dropbox/Local%20Development/unlazy/node_modules/.pnpm/rollup@4.14.3/no…
│ at async Promise.all (index 0)

@johannschopplich
Copy link
Owner

Seems like a path resolving issue on Windows. I will have to check that further. I'm busy at the moment but will try to take a look into it in the next days. 🙋‍♂️

@chrispreisler
Copy link
Author

I am on Mac OS, but thanks so much for looking into it, when you have the time for it :)

@johannschopplich
Copy link
Owner

Can you please try again?

pnpm i && pnpm build
cd packages/nuxt
pnpm dev

@chrispreisler
Copy link
Author

Hey Johannes,

thanks for looking into it. I can now build everything, but when I start the dev server, I still get the error:

Pre-transform error: Failed to resolve import "unlazy" from "src/runtime/components/UnLazyImage.vue". Does the file exist?

I also have completely recloned the whole project, so every dep and everything ist up to date.

Do you have any more ideas?

Best regards
Chris

@johannschopplich
Copy link
Owner

There seems to be some path errors, but I can't reproduce them.

@chrispreisler
Copy link
Author

Got it to work. I had to change your calculcation of the current directory to __dirname:

    resolve: {
      alias: {
        // '@unlazy/core': `${resolve(currentDir, '../../core/src')}/`,
        // 'unlazy': `${resolve(currentDir, '../../unlazy/src')}/`,
        'unlazy': resolve(__dirname, '../../unlazy/src'),
        '@unlazy/core': resolve(__dirname, '../../core/src'),
      },
    },

johannschopplich added a commit that referenced this issue May 6, 2024
@johannschopplich
Copy link
Owner

Maybe the files couldn't be resolved because of the space in your paths. I have refactored to use fileURLToPath in each instance, which should fix your path issues. (Hopefully)

@chrispreisler
Copy link
Author

Hey Johannes, thanks for that. I started implemented the media feature, but experienced another bug, I think:

Right now we get a layout shift, when you specify only width/height. But this should we work, because I have tested with a normal img tag and there we don't get a layout shift.

For the implementation of the media attribute, it should be possible to only specify width/height, because we can have different width/height values for different sources and so we don't know the aspect ratio, which wie normally can set with style aspect-ratio.

Maybe you can clone my repo and then start the dev server to see it in action:
https://github.com/chrispreisler/unlazy/blob/main/packages/nuxt/src/runtime/components/UnLazyImage.vue
https://github.com/chrispreisler/unlazy/blob/main/packages/nuxt/playground/app.vue

The media implementation for itself is working without problems.

Image.Layout.Shift.Bug.mov

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

No branches or pull requests

2 participants