Add translations to improve accessibility for account/list checkboxes #2982
Conversation
…nd list (only de and en) * change text for all cart entries * use select_all_on_page in checkedout.phtml, list.phtml, bulk-action-buttons * select_item_storage_retrieval_request_cancel in storageretrievalrequests * use texts in myresearch aria-labels * use select_item_[context] in checkbox.phtml
There was a problem hiding this comment.
Thanks for getting this started, @RLangeUni! I noticed a couple of things that may need further attention.
demiankatz
changed the title
|
It also looks like the language files need to be re-ordered to meet project standards -- just run |
|
Hi @demiankatz , let me know if there are further changes desired for this PR, I will be responsible for further editing. |
|
Thanks, @elsenhans! I will give this another review as soon as time permits (hopefully in the next day or two) and will let you know what I find. |
There was a problem hiding this comment.
I had more time to look at this today, @elsenhans. I made a couple of small changes:
1.) I adjusted the language of one of the English strings to make it more clear.
2.) I adjusted both language files so that they add a new select_all_on_page string instead of changing the existing select_page translation. I think this will give us more flexibility and will also ensure that we obtain the translations we need for other languages.
I also still have some concerns that the more verbose checkbox controls may impact the layout of the screen in a negative way, but I haven't actually started this up and looked at it yet -- I wanted to get other review changes made before doing hands-on testing.
See below for a few specific comments about one file that seems to be incompletely developed. I think once we get that sorted out, I'll be ready to try this and see if that leads to further suggestions.
Thanks again for moving this forward!
There was a problem hiding this comment.
Thanks for the ongoing progress, @elsenhans. I did another more thorough review and noticed a couple of places where the old select_page translation has not been replaced yet -- see comments below.
I also took time to capture some screen shots of how this impacts various parts of the visual interface. I'm sharing them here so we can get some opinions from others about whether or not this causes problems.
BOOK BAG
Before:
After:
SEARCH RESULTS WITH BULK OPTIONS
Before:
After:
FAVORITES
Before:
After:
CHECKED OUT ITEMS
Before:
After:
...for the most part, I think this looks fine with the longer text, but the button wrapping in the book bag lightbox is perhaps not ideal. Might it make sense to set things up so that the checkbox and button list wrap to separate lines if they are too large to fit together, rather than having the buttons partially wrapped? I'd be interested in opinions from @crhallberg, @sturkel89 and others...
|
Thanks, @elsenhans -- I'm now just awaiting feedback from others in order to determine if any further action is needed before we merge this. I'll be sure to discuss it on next month's Community Call if we don't reach a resolution sooner than that. From my personal perspective, the only thing that might need more attention is the layout/wrapping of the cart controls. Do you have any opinion about that? |
|
I think the division into two lines is good. |
|
I like the two lines approach, although I think the checkbox would be better below the buttons, closer to the items. Another option is to widen the lightbox, since it looks like this issue will only occur in the lightbox. |
Is there responsive markup we could use for all of these controls so that they display side by side when space permits and the checkbox drops below when necessary? Seems like something that should be possible. Is the lightbox currently a fixed width? It seems like it would probably make sense to give it more room, though we'd have to be sure there were no unexpected side effects. That's probably a task for a separate PR, though! |
|
Widen the lightbox seems to be a good solution for me too. We are going to do that in a separate PR right? |
|
I agree, @elsenhans, if we want to widen the lightbox, we should probably do that in a separate PR. I think the only outstanding issue here is whether there's markup @crhallberg can recommend to help with the flow of the checkbox and button elements. |
|
@demiankatz @crhallberg Can we please name the open work and finish it either here or a new PR? There are other PRs waiting for this one to be completed. One task for a new PR is to widen the lightbox. |
|
@elsenhans, I agree that a separate PR to enlarge the lightbox is a good idea. I think the main other task is to add responsive markup to make the layout of checkboxes and buttons tidier -- that's the change I'm waiting for before approving and merging this. I did ask @crhallberg yesterday to please weigh in on this -- but I think he's still catching up on things following WOLFcon last week. :-) |
|
My mistake! I thought the screenshots were already marked up. |
| @@ -47,7 +47,7 @@ | |||
| <?php if ($purgeSelectedAllowed): ?> | |||
| <label> | |||
| <input type="checkbox" name="selectAll" class="checkbox-select-all"/> | |||
| <?=$this->transEsc('select_page')?> | |||
| <?=$this->transEsc('select_all_on_page')?> | |||
| </label> | |||
There was a problem hiding this comment.
It looks like the .checkbox on line 46 can be closed here instead of line... actually it may not be closed...
There was a problem hiding this comment.
The checkbox div is closed, but it is wrapped around not just the checkbox but also all of the buttons. This is consistent with the markup style in holds/list.phtml, myresearch/checkedout.phtml, myresearch/illrequests.phtml and myresearch/storageretrievalrequests.phtml as well. As a result, buttons do not wrap as a group, which is inconsistent with the desired behavior we're working on elsewhere. However, because these templates have comparatively few buttons, it is less likely to cause problems in these contexts. It is probably worth making our checkbox+button markup consistent everywhere, but I think fixing this immediately is less critical.
|
Thanks for the input, @crhallberg -- see my replies to your comments above. I think we have at least three approaches here: 1.) If it is really important to us to get the checkbox to wrap BELOW the buttons, instead of above the buttons, I think we'll have to do a lot of work on all of the templates referenced above. I'm not sure if that's worth the effort, though it would be nice to have. 2.) If we want to make all of our markup consistent, perhaps we should consider adopting the div + ul approach for all checkbox + button combinations. (Though if a different approach is more accessible, I'm certainly open to other options...). This is a moderate amount of work, especially if it causes any complications for the various myresearch templates that are currently implemented without a lot of structured markup around the buttons. 3.) If we want a bare minimum solution to fix the most serious visual problems and get this PR merged, maybe adjusting the cart template is all that's absolutely necessary (since I really think the long checkbox text in the cart looks bad without wrapping, but the other templates either already wrap or have enough space that wrapping is unlikely). We could then do follow-up work on option 1 or 2 as a follow-up in a separate PR, if we feel it would be beneficial. What do you think, @crhallberg and @elsenhans? Any preferences or further ideas? |
|
I'm going to get to implementing this tomorrow. |
|
@RLangeUni, could you give me permission to push to this branch? That may be simpler than creating a separate pull request. |
|
To save @RLangeUni some time, I've gone ahead and pushed @crhallberg's latest changes to this branch. Thanks, @crhallberg! |
There was a problem hiding this comment.
This looks good to me now! There's one small inconsistency at present: the "select all" checkbox wraps ABOVE the buttons in the cart lightbox, and BELOW the buttons everywhere else. However, this will be fixed soon in a follow-up PR to make the lightbox window larger, as previously discussed, so I don't mind having it momentarily inconsistent in dev for a couple of days while we wait for that to be finalized.
(new translations are only provided for de and en)
This is the "fourth PR" simple string / helper texts, derived from #2874.
TODO: