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

issue #825: Множество открытых окошек жалоб #826

Merged
merged 3 commits into from
Nov 29, 2017
Merged

issue #825: Множество открытых окошек жалоб #826

merged 3 commits into from
Nov 29, 2017

Conversation

ivnish
Copy link
Member

@ivnish ivnish commented Nov 26, 2017

@ivnish
Copy link
Member Author

ivnish commented Nov 26, 2017

@mbaev, @awd-studio, посмотрите код.

  1. при клике по иконке жалобы popover открывается и закрывается (toggle сохранен, можно заменить на show)
  2. при клике по "крестику" popover закрывается
  3. при клике по "outside" popover закрывается

Предлагайте, как можно оптимизировать (сократить) код

'data-trigger' => 'click',
'title' => $category,
'data-trigger' => 'manual',
'title' => $category . '<button type="button" class="btn-ticket-popover-close close">&times;</button>',
Copy link
Contributor

Choose a reason for hiding this comment

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

Very-very bad practice!
Добавь свой html-тэг ниже. Это будет генерировать не валидный html. Читай theme_html_tag.

Copy link
Member Author

@ivnish ivnish Nov 29, 2017

Choose a reason for hiding this comment

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

Мне нужно вставить кнопку именно в title. Иначе она появляется где угодно, но только не в нужном месте. Как это сделать правильно?

Copy link
Member Author

Choose a reason for hiding this comment

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

Воспользовался JS для этого

var target = $(e.target);
if (target.data('toggle') !== 'popover'
&& target.parents('[data-toggle="ticket-popover"]').length === 0
&& target.parents('.popover.in').length === 0
Copy link
Contributor

Choose a reason for hiding this comment

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

Читай closest jquery.

Copy link
Member Author

Choose a reason for hiding this comment

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

Поясни за closest и parents. closest лучше использовать, потому что он вернет только один подходящий элемент? Или почему ты рекомендуешь его использовать?

Copy link
Contributor

Choose a reason for hiding this comment

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

Да, верно. Ищет первый подходящий и возвращает. Работает побыстрей.

mbaev
mbaev previously approved these changes Nov 29, 2017
Copy link
Contributor

@mbaev mbaev left a comment

Choose a reason for hiding this comment

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

В целом, норм. Так, ремарки по мелочам.

$('.popover-title').append('<button type="button" class="close btn-ticket-popover-close">&times;</button>');
});
// Close ticket-popover on click outside OR on click close button
$('body').on('click', function(e) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Можно оставить так, но обычно здесь используют $(document).

Copy link
Member Author

Choose a reason for hiding this comment

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

Изменю, чтобы в голове отложилось тоже)

});
// Close ticket-popover on click outside OR on click close button
$('body').on('click', function(e) {
var target = $(e.target);
Copy link
Contributor

Choose a reason for hiding this comment

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

Про $target помнишь? Не обязательно, но считается хорошей практикой.

Copy link
Member Author

Choose a reason for hiding this comment

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

Да, запоминаю потихоньку :)

@mbaev mbaev merged commit 346266e into DrupalRU:dev Nov 29, 2017
@ivnish
Copy link
Member Author

ivnish commented Nov 29, 2017

"всего" в 3 коммита уложился, расту)

@mbaev
Copy link
Contributor

mbaev commented Nov 29, 2017

Надеюсь ты потестил работоспособность в разных браузерах.

@ivnish ivnish deleted the issue-825 branch November 30, 2017 06:15
@ivnish
Copy link
Member Author

ivnish commented Dec 1, 2017

Firefox и Chrome.

@mbaev
Copy link
Contributor

mbaev commented Dec 1, 2017

А IE?

@ivnish
Copy link
Member Author

ivnish commented Dec 1, 2017

На виртуалке, где я разрабатываю нет возможности поставить IE. И самое время спросить, а какую минимальную версию IE мы должны поддерживать?

@mbaev
Copy link
Contributor

mbaev commented Dec 1, 2017

Можем поддерживать IE11 и EDGE

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

Successfully merging this pull request may close these issues.

2 participants