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

Fixed: Checkin limit string translation for components #12409

Merged
merged 3 commits into from
Feb 1, 2023

Conversation

akemidx
Copy link
Collaborator

@akemidx akemidx commented Jan 24, 2023

Description

Translating the string informing users of the limit to how many components can be checked in.

  • Bug fix (non-breaking change which fixes an issue)

Checklist:

@what-the-diff
Copy link

what-the-diff bot commented Jan 24, 2023

  • Added a new line to the language file
  • Changed an existing line in the view file

Copy link
Owner

@snipe snipe left a comment

Choose a reason for hiding this comment

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

Small clarification that I'd like to see, but not a hill I'll die on. Let me know if you're interesting in switching it to a variable, otherwise I'll take it as is :)

@@ -12,4 +12,5 @@
'remaining' => 'Remaining',
'total' => 'Total',
'update' => 'Update Component',
'checkin_limit' => 'Amount checked in must be equal to or less than this amount'
Copy link
Owner

Choose a reason for hiding this comment

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

This would be a great place to use a variable. Our implementation in the blade is - sub-optimal (and that's my fault), because different languages might be represent values in different parts of a sentence.

Something like:

'checkin_limit' => 'Amount checked in must be equal to or less than :assigned_qty

and then in the blade change (below) something like:

{{ trans(admin/components/general.checkin_limit, ['assigned_qty' => '$component_assets->assigned_qty']) }}

It's a minor detail, but comes across a little "friendlier".

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yea, that's an easy change! Something like this will pop up in other places and future PRs as well

@snipe snipe changed the title FIXED: Checkin limit string translation for components Fixed: Checkin limit string translation for components Jan 25, 2023
@@ -42,7 +42,7 @@
<input type="text" class="form-control" name="checkin_qty" aria-label="checkin_qty" value="{{ old('assigned_qty', $component_assets->assigned_qty) }}">
</div>
<div class="col-md-9 col-md-offset-2">
<p class="help-block">{{ trans(admin/components/general.checkin_limit) }}: {{ $component_assets->assigned_qty }}</p>
<p class="help-block">{{ trans(admin/components/general.checkin_limit, ['assigned_qty' => '$component_assets->assigned_qty']) }}</p>
Copy link
Owner

@snipe snipe Feb 1, 2023

Choose a reason for hiding this comment

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

Looks like we still need enclosing apostrophes here tho:

{{ trans(admin/components/general.checkin_limit, ['assigned_qty' => '$component_assets->assigned_qty']) }}

should be

{{ trans('admin/components/general.checkin_limit', ['assigned_qty' => '$component_assets->assigned_qty']) }}

The key we present in the trans() bit should always be enclosed in apostrophes.

Copy link
Owner

Choose a reason for hiding this comment

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

And actually we wouldn't need them enclosed with the variable part.

{{ trans('admin/components/general.checkin_limit', ['assigned_qty' => $component_assets->assigned_qty]) }}

When we're passing a variable, enclosing it on quotes will cause it to return the literal string of $component_assets->assigned_qty, instead of the value of $component_assets->assigned_qty.

@snipe
Copy link
Owner

snipe commented Feb 1, 2023

Looks great, thanks!!

@snipe snipe merged commit 4a6250a into snipe:develop Feb 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants