chore(Hint, Tooltip): unify positioning #3370

SchwJ · 2024-03-12T04:34:00Z

Проблема

В рамках работы над задачами #606 (Hint: Странно себя ведет у края окна браузера) и #1610 (Hint: изменять положение если не влезает на экран) стало ясно, что в Hint и Tooltip выбор позиции элемента организован по-разному (в одинаковых условиях получается разное поведение) и, в обоих случаях не очень логично.

Решение

Изменим порядок, в котором должны проверяться позиции расположения Hint'а и Tooltip'а.

Алгоритм теперь таков:

Если была указана приоритетная позиция, то пытаемся разместить в нее,
Если не удалось - проверяем её "соседей" - позиции на той же стороне в порядке center, left, right (для сторон top и bottom) и middle, top, bottom (для сторон left и right).
Если они не подходят - проверяем остальные стороны в цикле (на картинке) начиная со следующей за стороной приоритетной позиции

внутри каждой стороны проверяем позиции с выравниванием в порядке center, left, right (для сторон top и bottom) и middle, top, bottom (для сторон left и right).

В результате цикл проверки позиций будет таков (если разрешены все возможные позиции):

Ссылки

#1696 -- Hint, Tooltip: унифицировать и изменить порядок выбора положения элемента при его "непомещении" в приоритетную позицию

Чек-лист перед запросом ревью

Добавлены тесты на все изменения
⬜ unit-тесты для логики
⬜ скриншоты для верстки и кросс-браузерности
⬜ нерелевантно
Добавлена (обновлена) документация
⬜ styleguidist для пропов и примеров использования компонентов
⬜ jsdoc для утилит и хелперов
⬜ комментарии для неочевидных мест в коде
⬜ прочие инструкции (README.md, contributing.md и др.)
⬜ нерелевантно
Изменения корректно типизированы
✅ без использования any (см. PR 2856)
⬜ нерелевантно
Прочее
⬜ все тесты и линтеры на CI проходят
✅ в коде нет лишних изменений
✅ заголовок PR кратко и доступно отражает суть изменений (он попадет в changelog)

zhzz

Обсудили в офлайне:

Не хватает тестов на фиче-флаг
Жестко форсить логику позиционирования не нужно
Возможно, стоит объединить фиче-флаги для хинта и попапа
Возможно, стоит унифицировать и проп pos для хинта и тултипа

# Conflicts: # packages/react-ui/.creevey/images/Hint/hint near screen edge/chrome.png # packages/react-ui/.creevey/images/Hint/hint near screen edge/firefox.png # packages/react-ui/components/Hint/Hint.tsx # packages/react-ui/components/Hint/__stories__/Hint.stories.tsx # packages/react-ui/lib/featureFlagsContext/FEATUREFLAGSCONTEXT.md # packages/react-ui/lib/featureFlagsContext/ReactUIFeatureFlagsContext.tsx

# Conflicts: # packages/react-ui/.creevey/images/Hint/Hints without wrapper around inline-block with 50% width/chrome2022.png # packages/react-ui/.creevey/images/Hint/Hints without wrapper around inline-block with 50% width/chrome2022Dark.png # packages/react-ui/.creevey/images/Hint/Hints without wrapper around inline-block with 50% width/firefox2022.png # packages/react-ui/.creevey/images/Hint/Hints without wrapper around inline-block with 50% width/firefox2022Dark.png # packages/react-ui/components/Hint/Hint.tsx

packages/react-ui/lib/featureFlagsContext/FEATUREFLAGSCONTEXT.md

SchwJ · 2024-04-01T07:32:11Z

packages/react-ui/components/Hint/Hint.tsx

@@ -84,7 +81,10 @@ export interface HintState {
  position: PopupPositionsType;
 }

-const Positions: PopupPositionsType[] = [
+/**
+ * @deprecated This variable will be removed in the next version because of applying popupUnifyPositioning feature flag.


При удалении фиче-флаг сущность с дефолтными позициями хинта станет не нужной

SchwJ · 2024-04-01T07:34:49Z

packages/react-ui/components/Hint/Hint.tsx

-type DefaultProps = Required<
-  Pick<HintProps, 'pos' | 'allowedPositions' | 'manual' | 'opened' | 'maxWidth' | 'disableAnimations' | 'useWrapper'>
->;
+type DefaultProps = Required<Pick<HintProps, 'manual' | 'opened' | 'maxWidth' | 'disableAnimations' | 'useWrapper'>>;


Дефолтные pos и allowedPositions теперь будут вычисляться в Popup'е, так как логика их определения одинакова в Hint'е и в Tooltip'е.

SchwJ · 2024-04-01T07:38:17Z

packages/react-ui/components/Hint/Hint.tsx

@@ -129,14 +125,13 @@ export class Hint extends React.PureComponent<HintProps, HintState> implements I

  private timer: SafeTimer;
  private theme!: Theme;
-  private featureFlags!: ReactUIFeatureFlags;
+  public featureFlags!: ReactUIFeatureFlags;


Модификатор доступа изменен на public, так к флагам нужен доступ из Hint-test.tsx для тестирования расчёта приоритетной и возможных позиций.

SchwJ · 2024-04-01T07:48:50Z

packages/react-ui/components/Hint/Hint.tsx

+          splitPosition.length === 2 &&
+          (splitPosition[1] === 'middle' || splitPosition[1] === 'center') &&
+          allowedPositions.indexOf(splitPosition[0] as PopupPositionsType) === -1
+        ) {


Сложная проверка, проверяет ситуации:

заданная позиция не находится среди возможных позиций

нормализованная заданная позиция не находится среди возможных позиций (нормализация преобразует 'top' в 'top center' и определит его среди набора возможных позиций ['top center', ...])

обрезанная центральная позиция не находится среди возможных позиций ("обрезка" значений с middle и center направлениями и нахождение их среди ['top', 'bottom', 'left', 'right'])

SchwJ · 2024-04-02T09:25:22Z

packages/react-ui/components/Tooltip/Tooltip.tsx

+  /**
+   * @deprecated This variable will be removed in the next version because of applying popupUnifyPositioning feature flag.
+   */
+  public static oldDefaultAllowedPositions = Positions;


Дефолтная приоритетная позиция и дефолтные возможные позиции теперь будут задаваться в Popup'е, так как логика их определения одинакова в Hint'е и в Tooltip'е.

SchwJ · 2024-04-02T09:31:58Z

packages/react-ui/components/Tooltip/Tooltip.tsx

+          splitPosition.length === 2 &&
+          (splitPosition[1] === 'middle' || splitPosition[1] === 'center') &&
+          allowedPositions.indexOf(splitPosition[0] as PopupPositionsType) === -1
+        ) {


Сложная проверка, проверяет ситуации:

заданная позиция не находится среди возможных позиций

нормализованная заданная позиция не находится среди возможных позиций (нормализация преобразует 'top' в 'top center' и определит его среди набора возможных позиций ['top center', ...])

обрезанная центральная позиция не находится среди возможных позиций ("обрезка" значений с middle и center направлениями и нахождение их среди ['top', 'bottom', 'left', 'right'])

SchwJ · 2024-04-02T09:47:02Z

packages/react-ui/components/Tooltip/Tooltip.tsx

+          allowedPositions.indexOf(splitPosition[0] as PopupPositionsType) === -1
+        ) {
+          throw new Error(
+            'Unexpected position ' + pos + ' passed to Tooltip. Expected one of: ' + allowedPositions.join(', '),


Логика проверки выносится в Hint и Tooltip, так как необходимо возвращать ошибку (которая в этой строке) с указанием элемента (Hint и Tooltip), к котором она возникла. Было бы неправильно указывать ошибку в Popup'е, если пользователь его не использовал.

SchwJ · 2024-04-02T09:50:55Z

packages/react-ui/internal/Popup/Popup.tsx

+          allowedPositions.indexOf(splitPosition[0] as PopupPositionsType) === -1
+        ) {
+          throw new Error(
+            'Unexpected position ' + pos + ' passed to Popup. Expected one of: ' + allowedPositions.join(', '),


Хотя в Hint'е и Tooltip'е реализована проверка, в Popup'е она тоже необходима, так как Popup может быть использован пользователем в рамках своего контрола.

SchwJ · 2024-04-02T09:54:49Z

packages/react-ui/internal/Popup/__tests__/Popup-test.tsx

@@ -217,4 +217,83 @@ describe('properly renders opened/closed states', () => {
    expect(innerContainer).toHaveLength(1);
    expect(innerContainer?.children()).toHaveLength(0);
  });
+
+  describe('test getPosAndPositions method', () => {


Тестов без флага нет, так как при отсутствии флага popupUnifyPositioning функция getPosAndPositions не вызывается, нам нечего здесь проверять.

zhzz

Обсудили в офлайне:

Не нужно менять механизм позиционирования. Лучше остаться на существующем, который работает через массив уже отсортированных по нужному приоритету позиций. Если пользователь передал позиции в другом порядку, используем его.
Валидацию на корректность переданных позиций не нужно делать в каждом компоненте. Достаточно только в том, который их реализует. И ошибку решили заменить на warning.
Не нужно тестировать одинаковую логику в разных компонентах, если она реализуется в третьем компоненте. Плюс, не нужно привязываться к деталям реализации компонентов (к классовости компонентов, к именам его внутренних полей). Часто, логику можно вынести из компонента в хелпер и тестировать отдельно.

# Conflicts: # packages/react-ui/lib/featureFlagsContext/FEATUREFLAGSCONTEXT.md

zhzz · 2024-04-26T05:58:28Z

Зафиксирую договоренности после очередного обсуждения в офлайне:

Полученный массив позиций от пользователя никак не меняем, следуем ему
Если пользователь передал короткий вариант желаемой позиции, то находим первую подходящую в массиве и используем
Всю общую логику уносим в Popup. В Hint и Tooltip ее не дублируем.

zhzz · 2024-05-30T12:06:24Z

Закрыли в пользу #3416

SchwJ force-pushed the hint-tooltip-unify-positioning branch from 85a8c28 to 0c3c8e8 Compare March 18, 2024 10:33

zhzz requested changes Mar 21, 2024

View reviewed changes

SchwJ added 5 commits March 25, 2024 16:26

fix(): after review, right files

cb729f7

fix(): after review, fix problems, no tests

eba0a10

chore(Hint): add featureflag and fix screenshots

03fbbb2

chore(Hint): fix screenshot

9fdbf84

chore(Hint): only necessary changes

9b290e2

SchwJ force-pushed the hint-tooltip-unify-positioning branch from 0c3c8e8 to 9b290e2 Compare March 27, 2024 13:12

SchwJ added 7 commits March 28, 2024 09:03

fix(): screenshots

45eb00a

fix(): fix error and screenshots

f3b95db

fix(): fix review comments

3be4e82

fix(): fix review comments

cce22b1

fix(): fix review comments

550b17a

SchwJ commented Apr 2, 2024

View reviewed changes

SchwJ and others added 2 commits April 3, 2024 10:16

chore(): fix screenshots

e22adad

Merge branch 'next' into hint-tooltip-unify-positioning

8b6cf83

zhzz requested changes Apr 5, 2024

View reviewed changes

SchwJ added 6 commits April 24, 2024 10:44

chore(): fix review comments

e2c7deb

chore(): fix lint error

e0299d7

chore(): fix lint errors

a441c85

Merge branch 'next' into hint-tooltip-unify-positioning

8eba83c

# Conflicts: # packages/react-ui/lib/featureFlagsContext/FEATUREFLAGSCONTEXT.md

chore(): add feature flag context to Tooltip

4b2a098

chore(): fix error in test

e53f6ed

SchwJ mentioned this pull request Apr 27, 2024

chore(Popup, Hint, Tooltip): unify positioning #3416

Merged

zhzz closed this May 30, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(Hint, Tooltip): unify positioning #3370

chore(Hint, Tooltip): unify positioning #3370

SchwJ commented Mar 12, 2024 •

edited

zhzz left a comment

SchwJ Apr 1, 2024

SchwJ Apr 1, 2024

SchwJ Apr 1, 2024

SchwJ Apr 1, 2024

SchwJ Apr 2, 2024

SchwJ Apr 2, 2024

SchwJ Apr 2, 2024

SchwJ Apr 2, 2024

SchwJ Apr 2, 2024

zhzz left a comment

zhzz commented Apr 26, 2024

zhzz commented May 30, 2024

chore(Hint, Tooltip): unify positioning #3370

chore(Hint, Tooltip): unify positioning #3370

Conversation

SchwJ commented Mar 12, 2024 • edited

Проблема

Решение

Ссылки

Чек-лист перед запросом ревью

zhzz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhzz left a comment

Choose a reason for hiding this comment

zhzz commented Apr 26, 2024

zhzz commented May 30, 2024

SchwJ commented Mar 12, 2024 •

edited