Skip to content

Conversation

@sba
Copy link
Contributor

@sba sba commented Jun 28, 2022

With this pull request the error message in the file app/Views/errors/html/production.php can be translated.

I have named the file Errors.php because the translation of app/Views/errors/html/error_exception.php could also be added later.

@kenjis
Copy link
Member

kenjis commented Jun 29, 2022

Thank you for sending a PR.

You must GPG-sign your work, certifying that you either wrote the work or otherwise have the right to pass it on to an open-source project. See Developer's Certificate of Origin.
See https://github.com/codeigniter4/CodeIgniter4/blob/develop/contributing/pull_request.md#signing

<meta name="robots" content="noindex">

<title>Whoops!</title>
<title><?= lang('Errors.whoops')?></title>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<title><?= lang('Errors.whoops')?></title>
<title><?= lang('Errors.whoops') ?></title>

<div class="container text-center">

<h1 class="headline">Whoops!</h1>
<h1 class="headline"><?= lang('Errors.whoops')?></h1>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<h1 class="headline"><?= lang('Errors.whoops')?></h1>
<h1 class="headline"><?= lang('Errors.whoops') ?></h1>

<h1 class="headline"><?= lang('Errors.whoops')?></h1>

<p class="lead">We seem to have hit a snag. Please try again later...</p>
<p class="lead"><?= lang('Errors.weHitASnag')?></p>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<p class="lead"><?= lang('Errors.weHitASnag')?></p>
<p class="lead"><?= lang('Errors.weHitASnag') ?></p>

@sba
Copy link
Contributor Author

sba commented Jun 29, 2022

Hi @kenjis
Thanks for checking my PR and the GPG links. I will try to GPG-sign my commit...never did this before. Should I close this PR and make a new, signed one?
And is the naming "Errors.php" acceptable or do you have a better suggestion?
I thought about another name in case welcome_message.php should be made translatable later - but I think it would be better to name it for this Welcome.php anyway.

@kenjis
Copy link
Member

kenjis commented Jun 29, 2022

Should I close this PR and make a new, signed one?

No. You can do git commit --amend, and force push.

@MGatner
Copy link
Member

MGatner commented Jun 29, 2022

This should go to 4.3 branch. I'm fine with this change but just to note two things:

  1. These files are already in app/ so can be modified for any display already.
  2. The new language file will require adoption by the translations repo before this will actually work.

@kenjis
Copy link
Member

kenjis commented Jun 29, 2022

Agreed. It seems better to go to 4.3.

@sba
Copy link
Contributor Author

sba commented Jun 29, 2022

  1. These files are already in app/ so can be modified for any display already.

Yes, of corse. But with lang() functions it's multilingual out of the box.

@sba
Copy link
Contributor Author

sba commented Jun 29, 2022

  1. The new language file will require adoption by the translations repo before this will actually work.

I will have a look to the translations repo and try to update the necessary things (tests etc.)

@MGatner
Copy link
Member

MGatner commented Jun 30, 2022

Thanks @sba. To clarify, this should work fine without the translations (it falls back to English), but any additions you can make to the translations repo to go along with it would be appreciated!

@ddevsr
Copy link
Collaborator

ddevsr commented Jul 1, 2022

With this PR, must include in :
https://github.com/codeigniter4/CodeIgniter4/blob/develop/app/Views/errors/html/error_404.php#L79

Sorry! Cannot seem to find the page you were looking for. can multilingual too

@kenjis kenjis added the enhancement PRs that improve existing functionalities label Jul 1, 2022
Copy link
Member

@kenjis kenjis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should go to 4.3 branch. Please change the base branch.

@sba sba changed the base branch from develop to 4.3 July 5, 2022 09:15
@sba sba requested a review from kenjis July 5, 2022 09:25
Copy link
Member

@kenjis kenjis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@kenjis
Copy link
Member

kenjis commented Jul 5, 2022

@sba
Copy link
Contributor Author

sba commented Jul 5, 2022

@sba You forgot to sign your commits. See https://github.com/codeigniter4/CodeIgniter4/blob/develop/contributing/pull_request.md#signing

Yes, I know, sorry! It's a crap, I've wasted hours on it and signing still doesn't work...... I'll try again, sometime.

Copy link
Member

@MGatner MGatner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! I like @ddevsr's suggestion to include the 404 error as well, but that could be a separate PR if you're done with this.

sba and others added 6 commits July 5, 2022 14:33
Done in the Github editor, should now be signed!
Done in the Github editor, should now be signed!
Revert Code Style
Copy link
Member

@kenjis kenjis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are many unrelated commits.
Please drop these.

@kenjis
Copy link
Member

kenjis commented Jul 5, 2022

@sba Thank you for GPG signing.

But unfortunately this PR branch has many unrelated commits.
You need to clean up the branch.

I recommend you create a new branch from 4.3,
See https://github.com/codeigniter4/CodeIgniter4/blob/develop/contributing/workflow.md#branching-revisited
and cherry-pick commits you made.
After that remove this PR branch develop, and rename the new branch to develop and force push.

Or it is easier just to create another PR.

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement PRs that improve existing functionalities

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants