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

fix(input): input maxlength calculate the correct length #2654

Merged
merged 5 commits into from
Jul 25, 2021

Conversation

wen-haoming
Copy link
Contributor

Please make sure these boxes are checked before submitting your PR, thank you!

  • Make sure you follow Element's contributing guide English | (中文 | Español | Français).
  • Make sure you are merging your commits to dev branch.
  • Add some descriptions and refer to relative issues for your PR.

@element-bot
Copy link
Member

@wen-haoming
Copy link
Contributor Author

fix #2653

@jw-foss
Copy link
Member

jw-foss commented Jul 24, 2021

I’ve left few comments, please address them before it gets merged, I am happy to approve this PR after all comments are resolved. Thank you for your contribution! 👍

@wen-haoming
Copy link
Contributor Author

I’ve left few comments, please address them before it gets merged, I am happy to approve this PR after all comments are resolved. Thank you for your contribution! 👍

It's fixed ~

})
const inputExceed = computed(() => {
// show exceed style if length of initial value greater then maxlength
return isWordLimitVisible.value && (textLength.value > upperLimit.value)
return isWordLimitVisible.value && (textLength.value > props.maxlength)
Copy link
Member

Choose a reason for hiding this comment

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

I found a potential bug though, the type of maxlength says it can be both string and number type, I think yo need to parse maxlength before comparison operation. WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is my opinion that

  • The maxlength only allows the number type
    maxlength: {
      type: Number,
    },
  • return isWordLimitVisible.value && (textLength.value > Number(props.maxlength) ) Compatible with past string types

if (props.maxlength) {
const sliceIndex = inputExceed.value ? textLength.value : props.maxlength
// Convert value to an array for get a right lenght
value = Array.from(value).slice(0, Number(sliceIndex)).join('')
Copy link
Member

Choose a reason for hiding this comment

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

Like here you used Number(index) to make sure that the value is number even though '5' > '4' holds true but that doesn't mean that we can do so.

@@ -222,6 +222,9 @@ export default defineComponent({
type: Object,
default: () => ({}),
},
maxlength: {
Copy link
Member

Choose a reason for hiding this comment

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

@wen-haoming but you made the type string acceptable here, so I thought you need to do a string parse before doing actual arithmetic operations

Copy link
Member

Choose a reason for hiding this comment

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

Since we do not want to break the old functionality as passing that to <input /> which accepts both string and number type. I do think that you need to perform a Number or parseInt for maxlength though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think so 😄

@jw-foss jw-foss merged commit d55ca77 into element-plus:dev Jul 25, 2021
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

3 participants