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

support slots syntax for component interpolation #685

Merged
merged 6 commits into from
Aug 12, 2019
Merged

Conversation

aavondet
Copy link
Contributor

Related to #668. In short, this PR applies vue slots syntax to the interpolation functionality, while supporting backwards compatibility too. Thank you @sirlancelot for providing the base code, I edited it a bit so it passes all past test cases, then wrote some new test cases too. I am still lacking test cases for scoped slots though.

@codecov-io
Copy link

codecov-io commented Aug 11, 2019

Codecov Report

Merging #685 into dev will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##              dev    #685      +/-   ##
=========================================
+ Coverage   96.19%   96.2%   +<.01%     
=========================================
  Files          10      10              
  Lines         789     790       +1     
=========================================
+ Hits          759     760       +1     
  Misses         30      30
Impacted Files Coverage Δ
src/components/interpolation.js 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 999782a...a83871d. Read the comment docs.

@kazupon
Copy link
Owner

kazupon commented Aug 12, 2019

@aavondet
Thank you for your PR!
good job! 👍

I will additionally commit (e.g docs, tests) the missing codes! 💪

@kazupon kazupon changed the title Vue slots syntax for interpolation w/ tests ⭐ new: slots syntax for component interpolation Aug 12, 2019
@kazupon kazupon changed the title ⭐ new: slots syntax for component interpolation support slots syntax for component interpolation Aug 12, 2019
@kazupon kazupon merged commit 71ca843 into kazupon:dev Aug 12, 2019
@HapLifeMan
Copy link

HapLifeMan commented Aug 21, 2019

Hey @aavondet, thanks for your PR!

I'm here because I'm using nuxt-i18n which is using this repo. I updated all the dependencies yesterday and I'm facing some issues in my local environment. This PR broke the previous behavior with the place property in some cases:

// Some translations for the example

{
    myAmazingPath: 'This is a {firstInterpolation} {secondInterpolation} interpolation.',
    firstInterpolation: 'very',
    secondInterpolation: 'good',
}

✅ 1+ slot interpolations

<i18n path="myAmazingPath">
    <span slot="firstInterpolation">{{ $t('firstInterpolation') }}</span>
    <span slot="secondInterpolation">{{ $t('secondInterpolation') }}</span>
</i18n>

// Displays: "This is a very good interpolation."

✅ 1 place interpolation

<i18n path="myAmazingPath">
    <span place="firstInterpolation">{{ $t('firstInterpolation') }}</span>
</i18n>

// Displays: "This is a very interpolation."

❌ 2+ place interpolations

<i18n path="myAmazingPath">
    <span place="firstInterpolation">{{ $t('firstInterpolation') }}</span>
    <span place="secondInterpolation">{{ $t('secondInterpolation') }}</span>
</i18n>

// Displays: "This is a interpolation."

In this last case, the string translation is displayed but neither first or second interpolation, even the {firstInterpolation} or {secondInterpolation}... Can you take a look at it? Thanks!

@aavondet
Copy link
Contributor Author

Hello @HapLifeMan, thank you for the comment! As of right now there is a similar test case in test/unit/interpolation.test.js called "place prop on all children" that is similar with 2 interpolations. The test currently is passing so I'm not quite sure what the problem is off the bat so I'll take a closer look later on.

@aavondet
Copy link
Contributor Author

@HapLifeMan The place attribute should be the key, surrounded by {}, in your resource text. In your case you have 'This is a {1} {2} interpolation.' so you want to use <span place="1">$t('firstInterpolation)</span> and <span place="2">$t('secondInterpolation')</span>. The reason why the single case worked is because the default when you only have one interpolation is to place it in the first spot even though you did not specify a valid place attribute. Hope this fixes your problem!

@HapLifeMan
Copy link

Hey @aavondet, I'm sorry I made a mistake in the translation example.

Of course this is like this in my code:

{
    myAmazingPath: 'This is a {firstInterpolation} {secondInterpolation} interpolation.',
    firstInterpolation: 'very',
    secondInterpolation: 'good',
}

@aavondet
Copy link
Contributor Author

@HapLifeMan Hello, you still need to change your test case to the following:

<i18n path="myAmazingPath">
    <span place="firstInterpolation">$t('firstInterpolation)</span>
    <span place="secondInterprolation">$t('secondInterpolation)</span>
</i18n>

The place attribute only tells you the location in your myAmazingPath that html tag will be placed, so you still need to place something inside the HTML tag or else nothing will show up.

@HapLifeMan
Copy link

HapLifeMan commented Aug 27, 2019

Hey @aavondet, yes of course I know that and it's how it's built. I was just rushed on the examples...

It was perfectly working on multiple environments before this update with one or many interpolations, then broke up after the release that includes this merge. So I don't think it's a coincidence.

@kazupon
Copy link
Owner

kazupon commented Aug 27, 2019

@HapLifeMan
Thank you for your feedback!

This PR has been merged.
Could you register a GitHub issue with minimum reproduction code, please?

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.

4 participants