Use components and icon helper with offcanvas, fix issues. #3521
Conversation
|
@EreMaijala, am I just checking to make sure that everything is identical between test and dev in all three themes? Or are there specific improvements that should be visible to me? I know that I'll have to set offcanvas = true in config.ini, and try it in English and in Hebrew/Arabic. Are there any other specific things you'd like me to look at? |
| return ''; | ||
| } | ||
| ?> | ||
| <a class="search-filter-toggle icon-link" href="#search-sidebar" data-toggle="vufind-offcanvas" title="<?=$this->transEscAttr('sidebar_expand') ?>"> |
There was a problem hiding this comment.
If the only difference between the two show templates is the href, would it make more sense to have a single component with an argument? Or am I missing some more significant difference between the two templates?
There was a problem hiding this comment.
The link text is also different. I thought about a common template for them, but decided to keep the templates independent. I can add a common template they both use, if you think that's clearer.
There was a problem hiding this comment.
In any case, if we keep them separate, it's easier for customization. But of course harder for maintenance.
There was a problem hiding this comment.
Do you think there's a likely use case where somebody customizes one of these but not the other? It seems more likely that the user would want the same button behavior in both cases. Making the template more generic not only helps with that but also reduces the amount of redundant hard-coded strings in the template code (since the link is generated two different ways, and the text is currently repeated in both cases).
If somebody really did want to customize this differently in each situation, it would be possible to add a conditional based on the href.
All of these things make me lean toward a single template with parameters instead of two separate templates. But if you have a really strong use case for two templates, I can be persuaded otherwise. :-)
themes/bootstrap3/templates/_ui/components/account-offcanvas-show.phtml
Outdated
Show resolved
Hide resolved
Everything else should be pretty much the same as before, but the alignment of the offcanvas account menu should be better in search history (when logged in). |
There was a problem hiding this comment.
I compared test vs. dev in all themes and in both kinds of languages.
Everything is identical between test and dev except for two kinds of things - one an improvement, and the other something that I think needs a small additional tweak.
IMPROVEMENT
The layout of the My Account area looks much better in test.
Dev:
Test:
TWEAK NEEDED
Arrows in right to left languages seem to be pointing the wrong way in test. At least, a different logic is applied in those languages than in left to right languages than is applied in dev. See what you think:
Dev in English:
The arrows say: "look this way!"
Dev in Hebrew:
The arrows say: "look this way!"
TEST in English matches dev in English.
TEST in Hebrew:
These arrows look weird to me.
What do you think? I'm submitting this review as "request changes," but if the change is deliberate and is approved by a Hebrew- or Arabic-speaker, then please feel free to disregard my comments. :-)
|
@sturkel89 Good catch! I failed to take RTL into account. Should be fixed now. @demiankatz Now the template is more complicated so I had no problem moving to a common base; refactored. :) |
This is a bit of continuation from #3484 to get us better prepared for #3222, but also fixes some layout issues with offcanvas menu search history.
The high number of changed files is because the offcanvas links were spread over quite a few files. I consolidated them to components to clean things up and make future work easier as well.