Можно/нужно улучшить это код?

Question

[godsplane]

btnPop.forEach(btn => {
    if (e.target == btn) {
      let getAttributePopUp = btn.getAttribute('data-trigger');
      let popup = document.querySelector(`[data-popup='${getAttributePopUp}']`)
      popup ? popup.classList.add('active') : null
    }
  })

Суть в том чтобы при клике на кнопку брать ее дата атрибут и открывать попап с таким же дата атрибутом.
Просто строка с data-popup="${var}" выглядит для меня колхозно в плане опредления дата атрибута элемента который нужно показывать.
Можно ли и нужно ли как то улучшить эту структуру?

Comments

[TheOnlyFastCoder2]

?

const popup = [];
btnPop.forEach( (btn,i) => {
    btn.onclick = _ => {
      popup[i] ? popup[i].classList.add("active") : null
    }
})

Answer 1

[Сергей Соколов]

в этом коде неправильно готовят используют data-атрибуты

Comments

[godsplane]

А что не правильно?
вы предлагаете использовать dataset?Но я же не могу с его помощью получить элемент в 4 строке.
Можно заменить dataset на getAttribute но суть же не поменяется.

[Сергей Соколов]

godsplane, по ссылке, которую вы не читали, есть ответ. Прочтите внимательно.

[godsplane]

Сергей Соколов, я читал ее, но я все еще не понимаю каким образом я могу получить элемент через dataset , если вы вообще об этом

[Сергей Соколов]

Внутри этого когда мало, что можно улучшить.

btnPop.forEach((btn) => {
  if (e.target !== btn) return;

  const popup = document.querySelector(`[data-popup='${btn.dataset.trigger}']`);
  if (popup) {
    popup.classList.add('active');
  }
});

[godsplane]

Сергей Соколов, мало что можно улучшить потому что он плох или потому что идеален?Как бы вы сделали попап ?

[TheOnlyFastCoder2]

godsplane, ну уж точно не как вы

[godsplane]

TheOnlyFastCoder2, а как...

[TheOnlyFastCoder2]

godsplane, может в дс ? на почту(в моем аккаунте в описании ) напишете ник дискорда , а там посмотрим .
Или скидывайте код в codepen , чтобы посмотреть всю реализацию вашего popup

[Сергей Соколов]

godsplane, код вырван из контекста: не видно всего прочего. И какова задача — максимально упростить для пишущих HTML создание попапов? - поэтому через data-атрибуты связываете кнопку с попапом?.

[godsplane]

Сергей Соколов, ну мне просто казалаась идентификация через data-attribute когда их много это именно то что нужно, а попапов может быть много.
Весь код выглядит так

document.addEventListener('click', function (e) {
  let btnPop = document.querySelectorAll('[data-trigger]')
  let allPopUps = document.querySelectorAll('.popup');
  if (!e.target.closest('.popup-wrapper') || e.target.closest('.close-btn')) {
    allPopUps.forEach(pop => {
      pop.classList.remove('active')
    })
  }
  btnPop.forEach(btn => {
    if (e.target != btn) return
    let popup = document.querySelector(`[data-popup='${btn.dataset.trigger}']`)
    popup ? popup.classList.add('active') : null

  })
})

[godsplane]

TheOnlyFastCoder2, написал

[TheOnlyFastCoder2]

godsplane, почему вы ищите popup по data attribute , у вас их несколько типов и всего-лишь один ?

[godsplane]

TheOnlyFastCoder2, несколько видов попапов

[Сергей Соколов]

godsplane, че-т усложнили, вроде бы. Одна кнопка — один попап? Или там возможны многие-ко-многим?

[TheOnlyFastCoder2]

godsplane, разные данные или интерфейс popup'ов ?

[godsplane]

Сергей Соколов, условно на странице 5 кнопкок, 3 из них открывают один попап, 2 остальные другой.
TheOnlyFastCoder2, что вы имеете ввиду под данными?Попапы похожи, просто в одном из них немного другой текст и дополнительное поле в форме

[TheOnlyFastCoder2]

godsplane, Ну тогда вы почти правильно написали

let activePopup;
btnPop.forEach( (btn) => {
    btn.onclick = _ => {
      const popup = document.querySelector(`.popupType.${btn.dataset.popup}`)
      if(popup) {
        popup.classList.add("active")
        activePopup = popup
      }
    }
})

function delPopUp () {
  activePopup.classList.remove("active")
}

[TheOnlyFastCoder2]

godsplane, Можно такой вариант использовать .
компоненты можно в отдельном файле хранить

const component = ({html,indicator}) => {
    const dom = new DOMParser().parseFromString(html, "text/html");
    return dom.querySelector(indicator)
}

const popups = {
   popupError : (text) => component({
    indicator: ".popup",
    html:`
        <div class="popup error">
            <div class="popu__wrapper">
               ${text}
            </div>
        </div>
    `
  }),
  popupSomthing:(text) => component({
    indicator: ".popup",
    html:`
        <div class="popup something">
            <div class="popu__wrapper">
                ${text}
                ..............
            </div>
        </div>
    `
})
}


btnPop.forEach( (btn,i) => {
    btn.onclick = _ => {
       popupParent.append(popups[btn.dataset.popupType]("data"))
    }
})

[TheOnlyFastCoder2]

Сергей Соколов, как те последний вариант ? хехехеххе

[Сергей Соколов]

TheOnlyFastCoder2, открыл попап, и гори всё огнём? ) Как закрывать-удалять бум?

[TheOnlyFastCoder2]

Сергей Соколов,

//добавить кнопку в компоненте 
<button onclick="delPopup();"> del <button/>

function delPopup() {
  parentPopup.children[0].remove()
}