html validation at catalog #4594
Conversation
| @@ -32,7 +32,7 @@ <h1>Merge duplicate editions</h1> | |||
|
|
|||
| {{ pager() }} | |||
|
|
|||
| {{ total }} items need to be merged ({{ easy }} easy)<p> | |||
| {{ total }} items need to be merged ({{ easy }} easy)</p> | |||
There was a problem hiding this comment.
this doesn't look right to me. I think the problem here is that the macro pager creates a p tag but doesn't xlose it. Why don't we change the pager method to accept a string
e.g. {{ pager( '{{ %s }} items need to be merged ({{ %s }} easy)'%(total, easy) }}
OR let's not open that p tag in the macro pager.
There was a problem hiding this comment.
okay working on it
|
Is there anything I can do to help with this one @ArunTeltia ? |
ArunTeltia
force-pushed
the
html_validation
branch
from
a642af9 to
234f0de
Compare
|
Can you have a look now |
| {% endmacro %} | ||
|
|
||
| {{ pager() }} | ||
|
|
||
| {{ total }} items need to be merged ({{ easy }} easy)<p> | ||
| {{ total }} items need to be merged ({{ easy }} easy) |
There was a problem hiding this comment.
should this entire line be inside a p? i'm wondering if the
at the end was meant to close the one inside the macro originally?
e.g.
<p> {{ total }} items need to be merged ({{ easy }} easy)</p>
| @@ -27,12 +27,12 @@ <h1>Merge duplicate editions</h1> | |||
| <a href="?page={{ i + 1 }}">{{ i + 1}}</a> | |||
| {% endif %} | |||
| {% endfor %} | |||
| <p> | |||
|
|
|||
ArunTeltia
force-pushed
the
html_validation
branch
from
234f0de to
1d72de1
Compare
|
What URI are you testing these changes on @ArunTeltia ? |
|
Actually I was not checking in any URI I had just created a manual list in a paper and I am just checking HTML validation in each page manually. |
Please do not do this in the future as this is a very important part of the code review process and can save everyone a lot of time. If you are not sure about things questions are encouraged. Particularly with any frontend change you'll want to know which page to test on before taking on a patch. Having looked at this myself, I'm now convinced this is dead code, so we've wasted a few hours on code that is not running anywhere. @cdrini @mekarpeles could you please clarify how merge feature works? From looking at the code, I'm pretty sure the merge feature doesn't use any templates and if that's true we should just delete them all. |
|
Unfortunately this does look like dead code :( Sorry for the run-around @ArunTeltia ! And sorry to you both that you had to spend time fixing dead code 😓 Looks like we can delete |
|
In general, any .html with |
|
Created issue here: #4722 |
|
No problem sir working on it |
Working on #2798
This pr is the continuation of #4284
Stakeholders
@mekarpeles @jdlrobson