Update twig + fix #1019 change content language url #1022
Conversation
…ngs in hierarchy/groups panel
|
Kudos, SonarCloud Quality Gate passed!
|
Codecov Report
@@ Coverage Diff @@
## master #1022 +/- ##
============================================
+ Coverage 60.32% 67.91% +7.58%
+ Complexity 1593 1583 -10
============================================
Files 32 32
Lines 4429 3890 -539
============================================
- Hits 2672 2642 -30
+ Misses 1757 1248 -509
Continue to review full report at Codecov.
|
Fixes #1027 by commenting out css file path
The `results` field, which contains the search results, was incorrectly called `vocabularies` in the swagger spec - looks like a copy-paste error.
…uage fix #1018 by explicitly declaring the UI locale in getDate()
Fix error in swagger spec for search method return value
…nt and the side bar
…mappings remove dead code: the renderPropertyMappingValues function is never used
…rties Add HTML classes for properties so they can be targeted in JS and CSS
Fix calculation of minimum time in feedback honeypot
…-have-a-headline_or_subject Issue1093 feedback form should have a headline or subject Fixes #1093
Upgrade to PHPUnit 8; bump required PHP to 7.2+; fix Travis tests on PHP 7.4
Could you please rebase this on current master then, @kouralex? It would make it easier to see the actual changes remaining to be merged. |
There was a problem hiding this comment.
I'm struggling a bit with this PR...
First the apparent problem: there are two merge conflicts against master (see detailed comments) and they should be fixed before this can even be properly tested.
Second, I have the feeling again that this PR tries to do too many things at once. It's not crystal clear why it has to be like that. It would be better to fix problems individually in very small increments. That makes it easier to review, and also to backtrack if new problems are detected later.
Third, the PR makes it apparent that we are constructing very complex URLs using Twig expressions. This seems wrong-headed, and we should try to do that in PHP code instead, using the link_url filter if possible and if not, perhaps extending it and/or adding new similar filters. This isn't directly the fault of this PR though, the problem has existed before and this just makes it more visible.
| {% endfor %} | ||
| {% else %} | ||
| {% for langcode in lang_list %} | ||
| <li><a href="{{ request.lang }}/{{ request.page }}?clang={{langcode}}{% if term %}&q={{ term }}{% endif %}{% if vocabs %}&vocabs={{ vocabs }}{% endif %}" class="lang-button" hreflang="{{ langcode }}">{{ langcode | lang_name(request.lang) }}</a></li> | ||
| <li><a href="{{ request.lang }}/{{ request.page -}} |
There was a problem hiding this comment.
I can't help feeling that this is the wrong way to construct URLs in Twig templates - we should use the link_url filter or something like it to construct the complex URLs in PHP code, not here with Twig expressions. This wasn't as apparent before this PR, since all the complexity was "hidden" within very long lines in the Twig templates!
There was a problem hiding this comment.
Yes, this is how it was already done in code. I only refactored it.
| @@ -536,9 +536,6 @@ $(function() { // DOCUMENT READY | |||
|
|
|||
| // Setting the language parameters according to the clang parameter or if that's not possible the cookie. | |||
| var search_lang = (content_lang !== '' && !getUrlParams().anylang && vocab !== '') ? content_lang : readCookie('SKOSMOS_SEARCH_LANG'); | |||
| if (vocab === '' && readCookie('SKOSMOS_SEARCH_LANG') === 'anything') { | |||
There was a problem hiding this comment.
I didn't quite understand why the cookie handling was moved from JS to PHP/Twig code. I'm sure there's a good reason but it wasn't apparent from the PR :)
There was a problem hiding this comment.
Twig is processed before it is handled to the user, JS afterwards. One can see changes happening after the page has loaded - it is not ready for use. I think this is also for better accessibility, don't you think so, too?
There was a problem hiding this comment.
You're probably right. Can you explain how this is different from a user perspective? Is there some user-facing problem with the JS approach that is fixed here?
|
Kudos, SonarCloud Quality Gate passed!
|
|
@osma I did a rebase. You may now test this PR with the usual procedure. One can look https://github.com/NatLibFi/Skosmos/compare/issue1019-fix-change-content-language-url for code changes - the rebase made all commits to be applied to this branch. |
|
Thanks for the rebase. Yeah, something went awry there - the diff is now huge. Thanks for the link to the branch diff. |
This PR firstly updates Twig to 2.12.x so that a better whitespace control is in use, see https://twig.symfony.com/doc/2.x/templates.html#whitespace-control for more information. Then, utilize that to break down one-liners into multiple lines so that errors can be more easily found and corrected.
Additionally, on the front page, only clang-supported vocabularies are referred to by the clang parameter.
Finally, remove some JavaScript code related to cookies in favor of Twig code and fix an issue with duplicate
anylang=onparameter-key setting.Fixes #1019, addresses #918