Skip to content

show field #606

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

show field #606

wants to merge 1 commit into from

Conversation

Shkarin
Copy link
Contributor

@Shkarin Shkarin commented Jun 13, 2017 •
edited
Loading

Addressed to #565

@php-coder
Copy link
Owner

Спасибо! Я пока не смотрел на код, но вижу, что я сломал сборку когда внес изменения в настройки трэвиса. Я откатил свой коммит. Можешь обновиться из последнего мастера и заребейзить свою ветку? Извиняюсь :-(

@Shkarin Shkarin force-pushed the gh565_hidden_field branch from 757a554 to 4b80731 Compare June 13, 2017 19:28
@mystamps-bot
Copy link

mystamps-bot commented Jun 13, 2017 •
edited
Loading

1 Message
📖 @Shkarin thank you for the PR! All quality checks have been passed! Next step is to wait when @php-coder will review this code

Generated by 🚫 Danger

@Shkarin Shkarin force-pushed the gh565_hidden_field branch from 042438c to b04c1cc Compare June 13, 2017 20:15
php-coder
php-coder requested changes Jun 13, 2017
View reviewed changes
src/main/webapp/WEB-INF/views/series/add.html Outdated
@@ -283,7 +283,7 @@ <h3 th:text="${#strings.capitalize(add_series)}">
</div>
</div>

<div class="form-group form-group-sm collapse js-issue-date" th:classappend="|${issueDateHasErrors ? 'has-error in' : ''} ${issueDateHasValues ? 'in' : ''}|">
<div class="form-group form-group-sm collapse js-issue-date in" th:classappend="|${issueDateHasErrors ? 'has-error in' : ''} ${issueDateHasValues ? 'in' : ''}|">
Copy link
Owner

Choose a reason for hiding this comment

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

Перемести, пожалуйста, класс in поближе к классу collapse (или даже лучше помести их в конец): class="form-group form-group-sm js-issue-date collapse in"

src/main/javascript/series/add.js Outdated
@@ -10,6 +10,8 @@ function initPage(suggestCountryUrl) {
return CatalogUtils.expandNumbers(val);
});
});

$(".collapse").collapse('hide');

$('.collapse').on('show.bs.collapse hide.bs.collapse', function toggleChevron() {
Copy link
Owner

Choose a reason for hiding this comment

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

А за-chain-ить их нельзя? Типа такого $('.collapse').collapse().on()?

Еще интересно как оно сейчас работает -- ты скрываешь раньше чем добавляется обработчик на это событие и значит этот обработчик не отрабатывает? Значит стрелочки не будут правильно отображены?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

за-chain-ить не знаю как, остальное проверил и запушил

src/main/javascript/series/add.js Outdated
@@ -10,6 +10,8 @@ function initPage(suggestCountryUrl) {
return CatalogUtils.expandNumbers(val);
});
});

$(".collapse").collapse('hide');
Copy link
Owner

Choose a reason for hiding this comment

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

Если за-chain-ить не удастся, то замени двойные кавычки на одинарные.

@php-coder
Copy link
Owner

Сегодня вспомнил следующее:

обрати внимание на

th:classappend="|${gibbonsHasErrors ? 'has-error' : } ${showCatalogsInfo ? 'in' : }|"

они здесь для того, чтобы не скрывать те поля в которых есть ошибка. Похоже, что эта задача не такая тривиальная и надо внимательно подумать/посмотреть как это исправлять без риска сломать то, что уже работает.

@php-coder
Copy link
Owner

Пока я склоняюсь к мысли, что с сервера нужно всегда возвращать видимые поля, а на клиенте скрывать, но не все а только те где нет ошибок.

@Shkarin Shkarin force-pushed the gh565_hidden_field branch 2 times, most recently from da72a85 to 011e5ad Compare June 18, 2017 16:50
php-coder
php-coder requested changes Jun 18, 2017
View reviewed changes
src/main/webapp/WEB-INF/views/series/add.html Outdated
@@ -279,11 +279,11 @@ <h3 th:text="${#strings.capitalize(add_series)}">

<div class="form-group js-collapse-toggle-header">
<div class="col-sm-offset-3 col-sm-5">
<span class="glyphicon glyphicon-chevron-right" th:class="${issueDateHasErrors or issueDateHasValues ? 'glyphicon glyphicon-chevron-down' : 'glyphicon glyphicon-chevron-right'}"></span>&nbsp;<a href="javascript:void(0)" id="specify-issue-date-link" data-toggle="collapse" data-target=".js-issue-date" th:text="#{t_specify_issue_date}">Specify date of release</a>
<span class="glyphicon glyphicon-chevron-down" th:class="${issueDateHasErrors or issueDateHasValues ? 'glyphicon glyphicon-chevron-down' : 'glyphicon glyphicon-chevron-right'}"></span>&nbsp;<a href="javascript:void(0)" id="specify-issue-date-link" data-toggle="collapse" data-target=".js-issue-date" th:text="#{t_specify_issue_date}">Specify date of release</a>
Copy link
Owner

Choose a reason for hiding this comment

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

Раз у нас теперь шеврон всегда открыт, то th:class нам не нужен.

src/main/webapp/WEB-INF/views/series/add.html Outdated
@@ -371,11 +371,11 @@ <h3 th:text="${#strings.capitalize(add_series)}">

<div class="form-group js-collapse-toggle-header">
<div class="col-sm-offset-3 col-sm-7">
<span class="glyphicon glyphicon-chevron-right" th:class="${showCatalogsInfo ? 'glyphicon glyphicon-chevron-down' : 'glyphicon glyphicon-chevron-right'}"></span>&nbsp;<a href="javascript:void(0)" id="add-catalog-numbers-link" data-toggle="collapse" data-target=".js-catalogs-info" th:text="#{t_add_catalogs_info}">Add information from stamps catalogues</a>
<span class="glyphicon glyphicon-chevron-down" th:class="${showCatalogsInfo ? 'glyphicon glyphicon-chevron-down' : 'glyphicon glyphicon-chevron-right'}"></span>&nbsp;<a href="javascript:void(0)" id="add-catalog-numbers-link" data-toggle="collapse" data-target=".js-catalogs-info" th:text="#{t_add_catalogs_info}">Add information from stamps catalogues</a>
Copy link
Owner

Choose a reason for hiding this comment

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

Аналогично, нужно убрать th:class.

src/main/webapp/WEB-INF/views/series/add.html Outdated
@@ -477,11 +477,11 @@ <h3 th:text="${#strings.capitalize(add_series)}">

<div class="form-group js-collapse-toggle-header" sec:authorize="hasAuthority('ADD_COMMENTS_TO_SERIES')">
<div class="col-sm-offset-3 col-sm-5">
<span class="glyphicon glyphicon-chevron-right" th:class="${#fields.hasErrors('comment') or addSeriesForm.comment != null ? 'glyphicon glyphicon-chevron-down' : 'glyphicon glyphicon-chevron-right'}"></span>&nbsp;<a href="javascript:void(0)" id="add-comment-link" data-toggle="collapse" data-target=".js-comment" th:text="#{t_add_comment}">Add comment</a>
<span class="glyphicon glyphicon-chevron-down" th:class="${#fields.hasErrors('comment') or addSeriesForm.comment != null ? 'glyphicon glyphicon-chevron-down' : 'glyphicon glyphicon-chevron-right'}"></span>&nbsp;<a href="javascript:void(0)" id="add-comment-link" data-toggle="collapse" data-target=".js-comment" th:text="#{t_add_comment}">Add comment</a>
Copy link
Owner

Choose a reason for hiding this comment

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

Убери th:class.

@php-coder
Copy link
Owner

Когда исправишь комментарии выше, проверь используется ли еще во вью переменная showCatalogsInfo, если уже нет, то тоже удали.

@Shkarin Shkarin force-pushed the gh565_hidden_field branch from 23f11db to 2b6c5d1 Compare June 19, 2017 16:23
@Shkarin
Copy link
Contributor Author

Shkarin commented Jun 19, 2017 via email

php-coder
php-coder requested changes Jun 19, 2017
View reviewed changes
Copy link
Owner

@php-coder php-coder left a comment

Choose a reason for hiding this comment

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

Проверил -- не просто так раньше была проверка на showCatalogsInfo -- похоже, что придется ее вернуть.

Как вопроизвести ошибку (при включенном JS):

  • открыть страницу
  • ввести категорию, количество марок, выбрать изображение
  • ввести -1 в любое из полей для каталога Michel
  • после этого форма отобразится снова и будет видно только поле для каталога Michel.

Теперь если нажать на "Add information from stamps catalogues", то это поле исчезнет (хотя должны появиться все остальные поля).

Таким образом, когда хотя бы в одном из полей у каталогов есть ошибка -- показывать нужно все поля.

@php-coder
Copy link
Owner

(Пока проверял как работает сайт при отключеном JS -- нашел 2 баги :) )

Итак, поскольку при отключенном JS ссылки "Specify date of release" и прочие не работают, то их нужно скрывать по умолчанию и отображать только при включенном JS.

php-coder
php-coder reviewed Jun 22, 2017
View reviewed changes
src/main/javascript/series/add.js Outdated
@@ -17,6 +17,8 @@ function initPage(suggestCountryUrl) {
.find('.glyphicon')
.toggleClass('glyphicon-chevron-down glyphicon-chevron-right');
});

$('.collapse').not(".has-error, .js-has-data").collapse('hide');
Copy link
Owner

Choose a reason for hiding this comment

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

To prevent flickering during page load, let's use hide() here instead of collape().

Copy link
Owner

Choose a reason for hiding this comment

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

Still here.

Copy link
Owner

Choose a reason for hiding this comment

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

Все еще не исправлено, к сожалению :(

Copy link
Owner

Choose a reason for hiding this comment

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

А если заменить collapse на hide, то все ломается... Сходу даже не знаю почему.

Copy link
Owner

@php-coder php-coder Aug 6, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Ясно -- при замене collapse на hide, не происходит событие и не вызывается один из обработчиков. Давай тогда оставим как есть.

Copy link
Owner

Choose a reason for hiding this comment

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

У меня никакие тесты не падают, так как все быстро происходит.

Я разобрался с hide() -- его действительно нельзя использовать тут, так как collapse работает иначе, не как hide. Collapse не делает невидимым элемент, а изменяет его высоту до 0.

Вот, как я переписал и как вроде бы работает:

-       $('.collapse').not('.has-error, .js-has-data').collapse('hide');
+       // emulates collapse('hide') but hides elements faster
+       $('.collapse').not('.has-error, .js-has-data').height(0).removeClass('in').trigger('hide.bs.collapse');

Попробуй у себя, поможет ли это пройти тесты (ну и на трэвисе тоже).

@Shkarin
Copy link
Contributor Author

Shkarin commented Jun 27, 2017 via email

@Shkarin
Copy link
Contributor Author

Shkarin commented Jun 29, 2017 via email

@php-coder
Copy link
Owner

Итак, поскольку при отключенном JS ссылки "Specify date of release" и прочие не работают, то их нужно скрывать по умолчанию и отображать только при включенном JS.

тогда получается то что я делал бесполезно, проще закрыть ветку тогда?

Нет, просто нужно к ним добавить display: none и с помощью JS, отображать при загрузке страницы.

@php-coder
Copy link
Owner

Что-то я даже не знаю как исправить, кроме как костылей нагородить

Предлагаю попробовать нагородить :) Силы еще есть или уже все?

@Shkarin
Copy link
Contributor Author

Shkarin commented Jul 9, 2017 via email

@Shkarin
Copy link
Contributor Author

Shkarin commented Jul 22, 2017 via email

@php-coder
Copy link
Owner

Привет, я вернулся

Привет! Отлично!!

Задача была раскрывать поля "Specify date of
release", "Add information from stamps catalogues" если JS отключен, а
сейчас ты предлагаешь скрывать эти поля, если JS отключен?

Ты упустил одну деталь из моего сообщения. Вот оно:

Итак, поскольку при отключенном JS ссылки "Specify date of release" и прочие не работают, то их нужно скрывать по умолчанию и отображать только при включенном JS.

Я имел ввиду не поля, а ссылки, которые до этого использовались, чтобы эти поля раскрывать. Логика простая -- если у пользователя отключен JS и он видит поле, то ссылка на то, чтобы поле скрыть/раскрыть ему ни к чему так как она все равно не будет работать.

@Shkarin Shkarin force-pushed the gh565_hidden_field branch 2 times, most recently from a13e940 to c0343d8 Compare July 22, 2017 20:05
@Shkarin
Copy link
Contributor Author

Shkarin commented Jul 24, 2017 via email

php-coder
php-coder reviewed Jul 30, 2017
View reviewed changes
src/main/java/ru/mystamps/web/Url.java Outdated
@@ -82,7 +82,7 @@
public static final String ADD_SERIES_WITH_COUNTRY_PAGE = "/series/add/country/{slug}";

// MUST be updated when any of our resources were modified
public static final String RESOURCES_VERSION = "v0.3.2";
public static final String RESOURCES_VERSION = "v0.3.3";
Copy link
Owner

Choose a reason for hiding this comment

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

Надо снова обновить версию.

php-coder
php-coder reviewed Jul 30, 2017
View reviewed changes
src/main/javascript/series/add.js Outdated
@@ -10,13 +10,17 @@ function initPage(suggestCountryUrl) {
return CatalogUtils.expandNumbers(val);
});
});

$(".js-collapse-toggle-header").show();
Copy link
Owner

Choose a reason for hiding this comment

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

Use single quotes.

Copy link
Owner

Choose a reason for hiding this comment

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

Подозреваю, что именно из-за этой строчки ничего и не работает -- когда я открываю страницу с отключенным джаваскриптом, то не вижу полей с датой, комментарием и для ввода номеров по каталогам.

php-coder
php-coder reviewed Jul 30, 2017
View reviewed changes
src/main/javascript/series/add.js Outdated

$('.collapse').on('show.bs.collapse hide.bs.collapse', function toggleChevron() {
$(this)
.prev('.js-collapse-toggle-header')
.find('.glyphicon')
.toggleClass('glyphicon-chevron-down glyphicon-chevron-right');
});

$('.collapse').not(".has-error, .js-has-data").collapse('hide');
Copy link
Owner

Choose a reason for hiding this comment

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

Use single quotes.

@Shkarin
Copy link
Contributor Author

Shkarin commented Aug 3, 2017 via email

@php-coder
Copy link
Owner

Эмм ... вот я отключаю джаваскрипт http://prntscr.com/g3ygk1 и я все вижу, у тебя не так? или ты про другое?

Yes, in my case, some fields invisible. Maybe it depends on how you turning off JS? Did you reload a page after that to ensure that changes were applied?

@Shkarin
Copy link
Contributor Author

Shkarin commented Aug 3, 2017 via email

@Shkarin
Copy link
Contributor Author

Shkarin commented Aug 3, 2017 via email

@Shkarin Shkarin force-pushed the gh565_hidden_field branch from c0343d8 to 0c5f981 Compare August 3, 2017 17:06
@php-coder
Copy link
Owner

Сейчас еще раз проверил и не смог воспроизвести, так что ты прав.

А как насчет остальных комментариев?

@Shkarin Shkarin force-pushed the gh565_hidden_field branch from 0c5f981 to 0ae6a98 Compare August 5, 2017 18:56
@Shkarin
Copy link
Contributor Author

Shkarin commented Aug 5, 2017 via email

@Shkarin Shkarin force-pushed the gh565_hidden_field branch from bd7d978 to d32c422 Compare August 5, 2017 19:40
@php-coder
Copy link
Owner

Завтра смерджу. Там есть конфликт, так как я в мастере тоже увеличил версию в файле Url -- если ты его исправишь сам, отлично, если нет, то я сам поправлю, чтобы не "дергать" тебя по пустякам.

@php-coder
Copy link
Owner

@Shkarin I see that tests constantly fail on TravisCI. Did they pass on your laptop?

@Shkarin
Copy link
Contributor Author

Shkarin commented Aug 20, 2017 via email

@Shkarin Shkarin force-pushed the gh565_hidden_field branch from d32c422 to e3692e1 Compare August 20, 2017 15:31
@coveralls
Copy link

Coverage Status

Coverage remained the same at 75.055% when pulling e3692e1 on Shkarin:gh565_hidden_field into 00d021f on php-coder:master.

@Shkarin Shkarin force-pushed the gh565_hidden_field branch from e3692e1 to 8c7d651 Compare August 20, 2017 15:43
@Shkarin
Copy link
Contributor Author

Shkarin commented Aug 20, 2017 via email

@php-coder
Copy link
Owner

@Shkarin У тебя локально тесты с такими же ошибками падают?

@Shkarin
Copy link
Contributor Author

Shkarin commented Aug 22, 2017 via email

@php-coder
Copy link
Owner

Есть ли еще там какие-либо ошибки выше? В частности интересуют логи инициализации спрингового контекста при старте приложения. Запускается ли команда приложение по команде mvn spring-boot:run? Работают ли тесты в мастере?

@Shkarin
Copy link
Contributor Author

Shkarin commented Aug 23, 2017 via email •
edited by php-coder
Loading

@Shkarin Shkarin force-pushed the gh565_hidden_field branch from 8c7d651 to cd54a7b Compare August 24, 2017 16:56
@Shkarin Shkarin force-pushed the gh565_hidden_field branch from cd54a7b to 9a614ac Compare August 24, 2017 16:57
@coveralls
Copy link

Coverage Status

Coverage remained the same at 75.055% when pulling 9a614ac on Shkarin:gh565_hidden_field into fde518b on php-coder:master.

@php-coder
Copy link
Owner

Ура! Тесты в Трэвисе прошли! У тебя они тоже падать перестали?

@Shkarin
Copy link
Contributor Author

Shkarin commented Aug 27, 2017 via email

@php-coder
Copy link
Owner

Эх не знаю но у меня mvn verify выдает ошибку

Сами ошибки были где-то выше, а это лишь результат выполнения команды.

@php-coder
Copy link
Owner

Merged in 4844a35 commit.

Thank you!

@php-coder php-coder closed this Aug 27, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@php-coder php-coder php-coder approved these changes

Assignees
No one assigned
Labels
resolution/merged
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Shkarin @php-coder @mystamps-bot @coveralls