Skip to content

Изменил правописание св-ва у ActionSheetItem#1517

Closed
SkyGopnik wants to merge 1 commit intoVKCOM:masterfrom
SkyGopnik:fix/actionSheetItem-interface
Closed

Изменил правописание св-ва у ActionSheetItem#1517
SkyGopnik wants to merge 1 commit intoVKCOM:masterfrom
SkyGopnik:fix/actionSheetItem-interface

Conversation

@SkyGopnik
Copy link
Copy Markdown

Было
image

Стало
image

@SkyGopnik SkyGopnik requested a review from a team as a code owner April 4, 2021 22:11
@codesandbox-ci
Copy link
Copy Markdown

codesandbox-ci bot commented Apr 4, 2021

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit efb05d8:

Sandbox Source
VKUI - default example Configuration

@ewgenius
Copy link
Copy Markdown
Contributor

ewgenius commented Apr 5, 2021

  1. Хоть правка только в правописании, но по факту это изменение публичного апи компонента, которое потребует апдейта во всех местах где используется и может вызвать потенциальные поломки в приложениях - по хорошему надо оставить autoclose c deprecated пометкой, и автоматически выставлять autoClose если используется старый проп + кидать warn в консоль
  2. Если обновляется проп, нужно обновить все доки и примеры где он используется
  3. Хорошо бы обосновать это изменение: на скрине автокомплит показывает пропсы которые никак не относятся к ActionSheetItem

@SkyGopnik
Copy link
Copy Markdown
Author

1 и 2 хорошо, 3 просто это намного удобнее для чтения, не кажется? Те же onClick/onChange и т.д не пишут onclick, onchange, так и с autoClose, два разных слова. Если брать в пример Material UI то там именно так
image

@SkyGopnik
Copy link
Copy Markdown
Author

Если устраивает, то я могу взять, но на счёт пометки deprecated не очень понял, нужен пример, если можно.

@ewgenius
Copy link
Copy Markdown
Contributor

ewgenius commented Apr 5, 2021

3 просто это намного удобнее для чтения, не кажется? Те же onClick/onChange и т.д не пишут onclick, onchange, так и с autoClose, два разных слова. Если брать в пример Material UI то там именно так

Я знаю как camelCase пишется) но это breaking change, и так просто это изменять не надо

Если устраивает, то я могу взять, но на счёт пометки deprecated не очень понял, нужен пример, если можно.

export interface ActionSheetItemProps ...
  /**
   * @deprecated use autoClose instead
   */
  autoclose?: boolean;
  autoClose?: boolean;

...

const autoClose = props.autoClose || props.autoclose;

Я бы вообще это оставил на следующий мажорный релиз, чтобы не городить ради одной буквы в названии. Да, есть неконсистентность в написании пропсов (это кстати не единственное место где autoclose, еще есть Alert), но фактических проблем тут никаких нет

@SkyGopnik
Copy link
Copy Markdown
Author

Я могу сделать и запулим в мажорный тогда, а не сейчас

@SkyGopnik
Copy link
Copy Markdown
Author

Я могу сделать и запулим в мажорный тогда, а не сейчас

В таком случае deprecated нужен? Или сразу без него?
Alert тогда тоже захвачу и посмотрю где ещё так

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.

5 participants