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(intl-phone-input): fix format outside value #311

Merged
merged 9 commits into from
Nov 3, 2022

Conversation

igorokFront
Copy link
Contributor

Исправлено форматирование номера в intl-phone-input, который передаётся снаружи компонента в props value

@changeset-bot
Copy link

changeset-bot bot commented Oct 19, 2022

🦋 Changeset detected

Latest commit: 721f5ac

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

This PR includes changesets to release 1 package
Name Type
@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

@coveralls
Copy link

coveralls commented Oct 19, 2022

Pull Request Test Coverage Report for Build 3375205641

  • 11 of 13 (84.62%) changed or added relevant lines in 1 file are covered.
  • 134 unchanged lines in 8 files lost coverage.
  • Overall coverage decreased (-1.8%) to 79.67%

Changes Missing Coverage Covered Lines Changed/Added Lines %
packages/intl-phone-input/src/component.tsx 11 13 84.62%
Files with Coverage Reduction New Missed Lines %
packages/select/src/components/base-select/Component.tsx 1 91.34%
packages/intl-phone-input/src/components/select/component.tsx 2 79.17%
packages/intl-phone-input/src/utils/calculateCaretPos.ts 2 80.0%
packages/input-autocomplete/src/autocomplete-field/Component.tsx 3 70.0%
packages/select/src/utils.ts 6 87.64%
packages/intl-phone-input/src/useCaretAvoidCountryCode.ts 7 24.44%
packages/select/src/components/virtual-options-list/Component.tsx 54 7.63%
packages/intl-phone-input/src/component.tsx 59 43.29%
Totals Coverage Status
Change from base Build 3366517160: -1.8%
Covered Lines: 6679
Relevant Lines: 7652

💛 - Coveralls

@core-ds-bot
Copy link
Collaborator

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

@core-ds-bot
Copy link
Collaborator

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

@igorokFront
Copy link
Contributor Author

Добавил детект флага после вставки значения извне

@core-ds-bot
Copy link
Collaborator

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

@core-ds-bot
Copy link
Collaborator

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

@core-ds-bot
Copy link
Collaborator

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

@core-ds-bot
Copy link
Collaborator

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


export const restoreContainerStyles = (container: HTMLElement) => {
const modalRestoreStyles = getModalStore().getRestoreStyles();

const index = modalRestoreStyles.findIndex(s => s.container === container);
const index = modalRestoreStyles.findIndex((s) => s.container === container);
Copy link
Contributor

Choose a reason for hiding this comment

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

Как-то линтер не правильно отработал. Должно было остаться без скобок. И остальные изменения тоже странные

Copy link
Contributor

Choose a reason for hiding this comment

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

Хотя, и у меня тоже стало также...

Copy link
Contributor

@v-gevak v-gevak left a comment

Choose a reason for hiding this comment

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

Откати все лишние изменения и подлей мастер ветку

@igorokFront
Copy link
Contributor Author

Откати все лишние изменения и подлей мастер ветку

готово

@core-ds-bot
Copy link
Collaborator

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

[countryIso2],
);

const formatedValue = useMemo(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Почему ты форматируешь значение внутри компонента? Так в инпут попадает отформатированное значение, а в стейте остается неотформатированное

Copy link
Contributor Author

Choose a reason for hiding this comment

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

форматирую внутри компонента, потому что тут задача уже решается внутри компонента. А если снаружи отформатировать и форматирование будет отличаться от внутреннего - будут скачки. Более того тут используется библиотека для форматирования, которая не используется в других приложениях банка. Это внутренняя задача компонента, если значение пришло не отформатированным - его нужно форматировать

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • страна не определится, если мы извне поменяем значение

Copy link
Contributor

Choose a reason for hiding this comment

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

Я понимаю, я о том, что так получается рассинхрон со внешним стейтом, такого быть не должно. У тебя во внешнем стейте будет номер +79991111111, а в инпуте +7 999 111-11-11. Хотя на onChange улетает отформатированный номер. Это нужно исправить - либо там и там отформатированный номер, либо всегда внутри компонента отформатированный, а во внешнем стейте нет. Сейчас рандом полный

Copy link
Contributor Author

Choose a reason for hiding this comment

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

выглядит как мажорные изменения. Что если при получении неотформатированного значения просто вызывать onChange с отформатированным номером? Тогда и внешний и внутренний стейт будут в норме

Copy link
Contributor

Choose a reason for hiding this comment

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

"Что если при получении неотформатированного значения просто вызывать onChange с отформатированным номером?" - вроде это сделать и нужно

Copy link
Contributor Author

Choose a reason for hiding this comment

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

спасибо, сразу не сообразил. Поправил

};
return targetCountry;
},
[countries, countryIso2],
Copy link
Contributor

Choose a reason for hiding this comment

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

На каждый рендер прилетает новое значение countries, смысла с этого useCallback тут нет. Соответственно useEffect ниже на каждый рендер вызывается

Copy link
Contributor

Choose a reason for hiding this comment

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

eslint-ом бы еще пройтись по файлу

Copy link
Contributor Author

Choose a reason for hiding this comment

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

а почему прилетает новое значение countries? из-за дефолтного значения с вызовом функции? для чего это сделано?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

и как это лучше разрулить? в голову приходит только убрать тут countries из зависимостей

Copy link
Contributor Author

Choose a reason for hiding this comment

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

или убрать вызов метода из дефолта пропса

Copy link
Contributor

Choose a reason for hiding this comment

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

Убрать все useCallback и из зависимостей useEffect убрать getCountryByNumber и отключить eslint - eslint-disable-next-line react-hooks/exhaustive-deps ))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

спасибо, поправил

@core-ds-bot
Copy link
Collaborator

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

@core-ds-bot
Copy link
Collaborator

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

@@ -468,6 +466,26 @@ export const IntlPhoneInput = forwardRef<HTMLInputElement, IntlPhoneInputProps>(
/* eslint-disable-next-line react-hooks/exhaustive-deps */
}, [value]);

useEffect(() => {
if (value && value.length > 1 && !value.includes(' ')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Было бы неплохо описать эту логику в комментах

@SiebenSieben SiebenSieben merged commit a32104b into master Nov 3, 2022
@SiebenSieben SiebenSieben deleted the fix/FASTPAY-1857 branch November 3, 2022 16:02
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.

None yet

8 participants