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-autocomplete): changed onInput type #1043

Merged
merged 1 commit into from
Feb 8, 2024

Conversation

v-gevak
Copy link
Contributor

@v-gevak v-gevak commented Dec 13, 2023

Что сделано

Изменен тип onInput коллбэка с (event: ChangeEvent | null, payload: {value:string}) => void на (value: string) => void

Зачем

Возникают проблемы с event === null

Разработчики часто завязываются именно на первый аргумент коллбэков, игнорируют второй

И если event пришел null, то непонятно какой из этого вывод должен сделать разработчик

Отсюда вытекает подобный код:

const handleChange = (event) => {
 if (event) {
  const { value } = event.target
  // do something with value
 }
}

но это совершенно не равнозначно коду (особенно в случае input-autocomplete)

const handleChange = (_, { value }) => {
  // do something with value
}

что само собой порождает ошибки

Copy link

changeset-bot bot commented Dec 13, 2023

🦋 Changeset detected

Latest commit: a02ae43

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages
Name Type
@alfalab/core-components-international-phone-input Major
@alfalab/core-components-input-autocomplete Major
@alfalab/core-components-intl-phone-input Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@core-ds-bot
Copy link
Collaborator

Собрана новая демка.

@core-ds-bot
Copy link
Collaborator

Собрана новая демка.

Copy link
Contributor

@reme3d2y reme3d2y left a comment

Choose a reason for hiding this comment

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

Надо бы оповестить ребят, чтобы они были готовы к мажорке. Ну и можно обсудить с ними, чтобы вопросов не было.

(event, payload) — вообще оказалось плохой идеей. Хотя это как раз пример, когда коллективно сделали ошибочный выбор :D

@coveralls
Copy link

Pull Request Test Coverage Report for Build 7813320069

  • -1 of 15 (93.33%) changed or added relevant lines in 4 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.003%) to 84.431%

Changes Missing Coverage Covered Lines Changed/Added Lines %
packages/international-phone-input/src/components/base-international-phone-input/Component.tsx 8 9 88.89%
Totals Coverage Status
Change from base Build 7800768595: -0.003%
Covered Lines: 9254
Relevant Lines: 10143

💛 - Coveralls

@core-ds-bot
Copy link
Collaborator

Собрана новая демка.

@v-gevak v-gevak merged commit af03229 into master Feb 8, 2024
8 checks passed
@v-gevak v-gevak deleted the fix/change_callback_type branch February 8, 2024 05:52
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

5 participants