Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[8.x] Remove redundant description & localize template #39928

Merged
merged 2 commits into from
Dec 8, 2021
Merged

[8.x] Remove redundant description & localize template #39928

merged 2 commits into from
Dec 8, 2021

Conversation

xanderificnl
Copy link
Contributor

Screen readers will announce the type of role the landmark is, in this case screen readers would say "Pagination Navigation Navigation" -- this PR fixes that. This should be of benefit to accessibility.

See also "Redundant descriptions" @ mdn: developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Roles/form_role

Off-topic: I'd like to thank the maintainers of Laravel. You guys rock!

Screen readers will announce the type of role the landmark is, in this case screen readers would say "Pagination Navigation Navigation" -- this commit fixes that.

See also "Redundant descriptions" @ mdn: https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Roles/form_role
Screen readers will announce the type of role the landmark is, in this case screen readers would say "Pagination Navigation Navigation" -- this commit fixes that.

See also "Redundant descriptions" @ mdn: https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Roles/form_role
@taylorotwell taylorotwell merged commit 608c430 into laravel:8.x Dec 8, 2021
@taylorotwell
Copy link
Member

Thanks for contributing to Laravel! ❤️

@aaronhuisinga
Copy link

aaronhuisinga commented Dec 14, 2021

@taylorotwell @xanderificnl This actually introduces a bug that breaks Tailwind pagination out of the box, and it should ideally be reverted.

When attempting to use Tailwind pagination, I'm seeing the following:
htmlspecialchars(): Argument #1 ($string) must be of type string, array given (View: /var/www/html/vendor/laravel/framework/src/Illuminate/Pagination/resources/views/tailwind.blade.php)

The line causing the issue:

<nav role="navigation" aria-label="{{ __('Pagination') }}" class="flex items-center justify-between">

This is due to Laravel shipping with a resources/lang/en/pagination.php file by default. This PR causes the array returned by the pagination language file to be passed into the __() method, which results in the parsing exception.

@xanderificnl
Copy link
Contributor Author

xanderificnl commented Dec 14, 2021

I'm using this exact template in my app, and not running into this issue. I doubt this PR is causing your issue.

The line you quoted is from the simple-tailwind.blade.php and the error describes tailwind.blade.php, are you sure you're looking at the right view?

You can easily test via tinker (php artisan tinker):

❯ sail art tink
Psy Shell v0.10.12 (PHP 8.1.0 — cli) by Justin Hileman
>>> __('Pagination')
=> "Pagination"

You really shouldn't be getting an array conversion when localizing strings.

EDIT: While I'm not running into problems -- it does go against best practices lined out in the docs, therefore I've submitted a PR to resolve this matter. Thanks for bringing this to my attention @aaronhuisinga !

@xanderificnl
Copy link
Contributor Author

As said, I'm not running into this issue, but I did check the docs and they do agree that __('Pagination') isn't a great idea when there's pagination.php.

I'll be submitting a PR shortly to resolve this matter by adding a new translation key for pagination.

@aaronhuisinga
Copy link

@xanderificnl I did indeed paste the change from the wrong file. I was experiencing the issue in tailwind.blade.php.

I'm really not sure why it was triggering the error for us and not for your installation. We were just running a pretty standard Laravel app using Sail for testing, and ran into the issue after updating the framework.

Thanks for taking the time to check this out and submitting an updated PR!

@xanderificnl
Copy link
Contributor Author

Hi.

I was just looking through my source and I have a file called resources/lang/en.json. Considering the documentation, that may be the reason why I haven't hit this issue.

Currently in the train w/o access to a machine besides my phone so I can't actually verify it by trying it out.

Thanks again for bringing this to my attention!

Cheers,

  • Xander

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants