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(tabs): add fallback values for element width values #479

Merged
merged 3 commits into from
Feb 8, 2023

Conversation

mrAnomalyy
Copy link
Contributor

@mrAnomalyy mrAnomalyy commented Jan 23, 2023

Опишите проблему

При тестировании доработки collapsible вкладок на проекте (ссылка на ПР), обнаружили некорректное поведение вкладок в браузере Mozilla Firefox. Хук useCollapsibleTabs некорректно составлял список вкладок, котроые должны быть сколлапсированы. Кроме того, в некоторых случаях, при переключении между вкладками, размер элемента тайтла вкладки некорректно возвращался, из-за чего вкладки также переставали коллапсировать

Шаги для воспроизведения

  1. Перейти на страницу используя Mozilla Firefox

Этот кейс появился из-за изменений в вышеуказанном ПРе, там переиспользовался тэг role="tablist", что создавало два элемента с такой ролью и мешало найти правильный элемент (почему-то в Safari и Chrome ничего такого не воспроизводится, что не дало отследить багу в процессе разработки).

  1. Перейти на демку и выбрать тип вкладок Collapsible
  2. Снять все галочки с precollapsible вкладок
  3. Выбрать вкладку из "Ещё"
  4. Повторять пункт 3, пока не наткнётесь на баг - определенной закономерности выявить не получилось.

Что происходит: в хуке useCollapsibleTabs некорректно определяется width элементов вкладок, браузер постоянно возвращает ширину 0, из-за этого ни одна вкладка не считается сколлапсированной и вкладки уходят за overflow элемента контейнера.

Решение, предложенное в ПР: использовать fallback магическое число 100, в случае, когда не получается определить ширину элемента.

Тестовый стенд

Десктоп (если данных нет оставте блок пустым):

  • OS: MacOS
  • Browser: Safari
  • Version: 16.2
    --
  • OS: MacOS
  • Browser: Google Chrome
  • Version: 109
    --
  • OS: MacOS
  • Browser: Firefox
  • Version: 108.0.2

Дополнительная информация

Решение, надо сказать такое себе. Буду рад услышать предложения и идеи, или увидеть Вашу реализацию фикса этой проблемы.

P.S. была идея кэшировать ширину элементов, пока она получается браузером и использовать кэшированные значения, когда браузер начинает возвращать 0.
P.S.S. Попытка засунуть функцию в которой происходит получение ширины в таймаут не сработала

@changeset-bot
Copy link

changeset-bot bot commented Jan 23, 2023

⚠️ No Changeset found

Latest commit: 940196f

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

@coveralls
Copy link

coveralls commented Jan 23, 2023

Pull Request Test Coverage Report for Build 4101906207

  • 4 of 4 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.008%) to 80.478%

Totals Coverage Status
Change from base Build 4081717002: -0.008%
Covered Lines: 7389
Relevant Lines: 8363

💛 - Coveralls

@v-gevak
Copy link
Contributor

v-gevak commented Jan 24, 2023

@CrybabyDanChan посмотри плиз

@DemonicCodeSlayer
Copy link
Contributor

Снимок экрана 2023-01-25 в 10 14 09

Вот такое словил, после фикса как то некорректно работает управления схлопнутыми вкладками

@mrAnomalyy
Copy link
Contributor Author

@CrybabyDanChan это Firefox? Есть кейс?

@DemonicCodeSlayer
Copy link
Contributor

@CrybabyDanChan это Firefox? Есть кейс?

Пытался записать скринкаст, сложно уловить кейс(хотя в первый раз попался мне сразу), это в firefox

  1. Перейти на демку и выбрать тип вкладок Collapsible
  2. Выбрать все галочки с precollapsible
  3. Понажимать еще раз на любые галочки из precollapsible
  4. Снять все галочки с precollapsible вкладок

Сейчас воспроизводится довольно редко, скорей всего особенности мозилы, в safari и chrome тестил, баг не воспроизвелся, скорей всего это не критично...

@mrAnomalyy
Copy link
Contributor Author

@CrybabyDanChan я не смог это воспроизвести

Голубев Богдан Владимирович added 2 commits February 6, 2023 12:13
@mrAnomalyy
Copy link
Contributor Author

mrAnomalyy commented Feb 6, 2023

Использовал scrollWidth вместо offsetWidth, как посоветовал @Lacronts. С ним, вроде бы, всё работает как нужно, в т.ч. в Firefox 🥸

@core-ds-bot
Copy link
Collaborator

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

@mrAnomalyy
Copy link
Contributor Author

Проверил демку. Вроде бы всё в порядке.

@mrAnomalyy mrAnomalyy requested a review from v-gevak February 6, 2023 12:07
@v-gevak v-gevak merged commit 820dcf0 into master Feb 8, 2023
@v-gevak v-gevak deleted the fix/collapsible-tabs branch February 8, 2023 10:46
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.

6 participants