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

⚡ improvement(interpolation): enable passage of local components to tag prop #928

Merged
merged 1 commit into from Jul 29, 2020

Conversation

vhoyer
Copy link
Contributor

@vhoyer vhoyer commented Jul 1, 2020

This should enable the user to, not only pass global components and html tags, but also locally imported components.


Of note: I commited this changes directly on github web interface. So I didn't write tests or any lint on commit, I did so because I want to just test the ideia with the community/mantainers, if you like the idea I'm more than willing to fix any mistakes I may have commited.

@kazupon
Copy link
Owner

kazupon commented Jul 25, 2020

Thank you for your PR!

dev branch is reserved for vue-i18n next major dev brach.
8.x branch is maintained for Veu 2.x.
You can check it!

The current specifications of i18n component is here:
https://kazupon.github.io/vue-i18n/api/#i18n-functional-component

In about this PR, We need your more detail explanation and use-case.
Could you tell us about these, please?

@kazupon kazupon added the Status: Need More Info Lacks enough info to make progress label Jul 25, 2020
@vhoyer vhoyer changed the base branch from dev to v8.x July 28, 2020 13:10
@vhoyer vhoyer changed the base branch from v8.x to dev July 28, 2020 13:11
@vhoyer
Copy link
Contributor Author

vhoyer commented Jul 28, 2020

Yeah sure, I was motivated to do this, because on the company I work for, we have a text component (to standardize text styles between various projects), when this text component is not a global component I can't use it with i18n. Like:

Currently on vue-i18n@8.x I can't do this:

<template>
  <i18n :tag="$options.UiText" path="a.path" />
</template>

<script>
import UiText from 'file'
export default {
  UiText,
}
</script>

This PR makes ⬆️ that example work. A.K.A.: This PR makes i18n be able to render locally imported components instead of only global ones 😄


Alternatively, on the current live version, one would have to do the following to achieve the same result:

<template>
  <UiText>
    <i18n :tag="false" path="a.path" />
  </UiText>
</template>

<script>
import UiText from 'file'
export default {
  components: { UiText },
}
</script>

@kazupon
Copy link
Owner

kazupon commented Jul 28, 2020

Thank you for your explanation!
I've understood this feature.

We need unit test for this feature and should change branch to v8.x.

Could you update this PR, please?

@vhoyer
Copy link
Contributor Author

vhoyer commented Jul 28, 2020

Sure, will do this later today :D

@vhoyer vhoyer changed the base branch from dev to v8.x July 29, 2020 01:37
@codecov-commenter
Copy link

Codecov Report

Merging #928 into v8.x will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             v8.x     #928   +/-   ##
=======================================
  Coverage   96.42%   96.42%           
=======================================
  Files          10       10           
  Lines         896      896           
=======================================
  Hits          864      864           
  Misses         32       32           
Impacted Files Coverage Δ
src/components/interpolation.js 100.00% <ø> (ø)

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 6304c34...64d5e1e. Read the comment docs.

@kazupon kazupon added the Type: Feature Includes new features label Jul 29, 2020
Copy link
Owner

@kazupon kazupon left a comment

Choose a reason for hiding this comment

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

Thanks!
LGTM! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: merged Status: Need More Info Lacks enough info to make progress Type: Feature Includes new features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants