Conversation
janno42
left a comment
janno42
left a comment
There was a problem hiding this comment.
all changes from the commit "fixes to the pr" should be squashed into the two commits before.
also, the "add tests" commit should optimally be splitted into one naming the forms and the other one adding the actual tests for this feature.
| class Migration(migrations.Migration): | ||
|
|
||
| replaces = [('information_pages', '0004_auto_20180813_2050'), ('information_pages', '0005_auto_20180813_2102')] | ||
|
|
There was a problem hiding this comment.
this is a squashed migration although that's not necessary because both 0004 and 0005 didn't exist before. please create a plain new migration instead.
| class Migration(migrations.Migration): | ||
|
|
||
| replaces = [('minutes', '0010_auto_20180813_2050'), ('minutes', '0011_auto_20180813_2102')] | ||
|
|
| <a href="#" id="popover" data-placement="bottom"> | ||
| <span class="glyphicon glyphicon-search" aria-hidden="true"></span> | ||
| </a> | ||
| <div id="popover-content" class="hide"> |
There was a problem hiding this comment.
the indentation here is wrong
| {% endif %} | ||
| {# Search form #} | ||
| <li class ="pull-right"> | ||
| <a href="#" id="popover" data-placement="bottom"> |
There was a problem hiding this comment.
please make the popover dismissable (http://getbootstrap.com/docs/4.1/components/popovers/#dismiss-on-next-click)
| </ul> | ||
| </li> | ||
| {% endif %} | ||
| {# Search form #} |
There was a problem hiding this comment.
please add some javascript that automatically focuses the search text field on opening the popover
| $('[data-toggle="popover"]').popover() | ||
| }); | ||
|
|
||
| $('#popover').popover({ |
There was a problem hiding this comment.
can you combine these two method calls into one?
3 participants
reopened from #656, there are some open issues here.
i will add some review comments.