Skip to content
This repository has been archived by the owner on Apr 25, 2024. It is now read-only.

refactored two img elements to one #39

Closed
wants to merge 1 commit into from
Closed

refactored two img elements to one #39

wants to merge 1 commit into from

Conversation

Udassi-Pawan
Copy link

Resolves #34

Summary of Changes

I refactored the two img elements to just one conserving the styles.
I am relatively newer to open source contributions. Do let me know if any changes are required.

@@ -4,7 +4,9 @@ export default class EventDetails extends HTMLElement {
this.innerHTML = `
<div class="inline-block event-details">

<img src="/assets/images/ri-map.webp" class="hidden sm:block max-w-sm md:max-w-sm lg:max-w-md xl:max-w-md" style="position:absolute; z-index:-1">

<img src="/assets/images/ri-map.webp" class="block pt-2 w-350 sm:pt-0 sm-max-w-sm md:max-w-sm lg:max-w-md xl:max-w-md sm:absolute sm:z-(-1)">
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<img src="/assets/images/ri-map.webp" class="block pt-2 w-350 sm:pt-0 sm-max-w-sm md:max-w-sm lg:max-w-md xl:max-w-md sm:absolute sm:z-(-1)">
<img src="/assets/images/ri-map.webp" class="block pt-2 w-[350px] sm:pt-0 sm-max-w-sm md:max-w-sm lg:max-w-md xl:max-w-md sm:absolute sm:z-(-1)">

Copy link
Contributor

@Arindam200 Arindam200 Apr 23, 2023

Choose a reason for hiding this comment

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

Hey @Udassi-Pawan , I think w-350 will not work. Instead, you can use w-[350px]

@@ -4,7 +4,9 @@ export default class EventDetails extends HTMLElement {
this.innerHTML = `
<div class="inline-block event-details">

<img src="/assets/images/ri-map.webp" class="hidden sm:block max-w-sm md:max-w-sm lg:max-w-md xl:max-w-md" style="position:absolute; z-index:-1">

<img src="/assets/images/ri-map.webp" class="block pt-2 w-350 sm:pt-0 sm-max-w-sm md:max-w-sm lg:max-w-md xl:max-w-md sm:absolute sm:z-(-1)">
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<img src="/assets/images/ri-map.webp" class="block pt-2 w-350 sm:pt-0 sm-max-w-sm md:max-w-sm lg:max-w-md xl:max-w-md sm:absolute sm:z-(-1)">
<img src="/assets/images/ri-map.webp" class="block pt-2 w-350 sm:pt-0 sm-max-w-sm md:max-w-sm lg:max-w-md xl:max-w-md sm:absolute sm:z-[-1]">

Copy link
Contributor

@thescientist13 thescientist13 left a comment

Choose a reason for hiding this comment

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

Thanks @Udassi-Pawan for this!

I realize I forgot to leave out an important detail in the issue 🤦‍♂️

So for mobile, we needed to use a second image where the dots keep going down to the logo just because we wanted to keep everything stacked and narrow for these kind of screen sizes. I have a PR up for this now that I got the latest image from the designer - #40
Screen Shot 2023-04-24 at 7 10 11 PM

This means that we would still need to manage two different src attributes based on breakpoint. I'm thinking we could use srcset to select which image to load, while still keeping the styles all in Tailwind? Hopefully that allows us to still use one <img> tag. 🤞

Let me know your thoughts, and apologies for the missing details.

@thescientist13 thescientist13 added the enhancement New feature or request label Apr 24, 2023
@thescientist13
Copy link
Contributor

thescientist13 commented Apr 29, 2023

Just wanted to share an example that I just implemented for the header banner image that you can reference
https://github.com/AnalogStudiosRI/www.blissfestri.com/blob/main/src/components/header/header.js#L9

  <img
    src="/assets/images/blissfest-logo-sm.webp"
    srcset="/assets/images/blissfest-logo-sm.webp 500w,
            /assets/images/blissfest-logo-md.webp 1000w,
            /assets/images/blissfest-logo-lg.webp 1500w"
  />

In this case I did the following:

  1. Set the src to the default image to show, which I choose to be to target mobile breakpoints as the default
  2. Create an entry in the source set at each breakpoint I wanted to target; 350px, 1000px, 1500px across three different screen sizes.

So to apply that to this instance, I think something like this should probably work

<img
  src="/assets/images/ri-map-mobile.webp"
  srcset="/assets/images/ri-map-mobile.webp 350w,
          /assets/images/ri-map.webp
/>

@Udassi-Pawan Udassi-Pawan closed this by deleting the head repository Aug 12, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

refactor double usage of <img> tags for event details component
3 participants