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

tech(Flex): add support flex with gap #8212

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

EldarMuhamethanov
Copy link
Contributor

@EldarMuhamethanov EldarMuhamethanov commented Jan 30, 2025

  • e2e-тесты

Описание

На данный момент мы не можем использовать свойство gap в flex контейнере - наша браузерная поддержка не покрывает поддержку этого свойства совместно с display: flex. Поэтому в компоненте Flex для реализации gap используются хаки с margin-ами. Хотелось бы использовать свойство gap как основную реализацию, и если она не поддерживается, то использовать фолбэк с текущей реализацией. Нужно придумать как это можно сделать используя только css.

Исследования

Первым делом попробовал использовать @support (gap: 1px) для проверки поддержки gap. Как оказалось этот способ не работает, так как само свойство появилось раньше. А его поддержка в flex контейнере появилась позже

Нашел issue на github в котором обсуждали эту проблему. В одном из комментариев пользователь предложил следующий способ:
Можно использовать @support (inset: 0), так как его браузерная поддержка максимально близка к поддержке gap + flex
Поддержка gap + flex
image
Поддержка inset
image

Из этого можно сделать вывод: @support (inset: 0) не будет срабатывать на все браузеры, которые не поддерживают flex + gap. Но также и не будет срабатывать для нескольких версий браузеров, где gap уже поддерживается, а именно: Chrome 84 - 86, Edge 84 - 86, Firefox 63 - 65, Opera 70 - 72.

Решение есть, осталось только определить плюсы и минусы данной доработки:

Плюс и минусы

+ У большинства пользователей будет использоваться нативное решение, у которого не будет багов, которые есть у фолбек решения, такие как вложенные Flex и тд.
- Разработчикам придется поддерживать в будущем(пока полностью не откажемся от фолбека) два подхода. Но я бы не сказал, что это будет проблемно, учитывая, что кода в целом то немного. Плюс небольшая вероятность, что в компонент Flex будет добавляться много функционала.

Немного статистики

Есть страничка со статистикой использования браузеров и их версий на caniuse. Исходя из нее можно сделать вывод, что очень мало пользователей пользуется браузерами, которые еще не поддерживают gap

Еще сайт со статистикой по браузерам

Кейсы, которые будут работать в новой реализации

Если прокинуть в children один Fragment, в котором будет несколько children
Реализация через gap:
image
В fallback реализации не будет выставлен:
image

Это происходит из-за этого кода:
image

Данный код правит баг из #7463. Можно, конечно, рекурсивно рассчитывать настоящее количество child-ов, но скорее всего это может ударить по производительности

В новой реализации обоих этих багов не будет

Изменения

  • Добавил использование gap если поддерживается свойство inset
  • Поправил код, чтобы размеры gap рассчитывались всегда, даже, когда 1 children. Это исправляет проблему, когда в children прокинут Fragment с множеством child-ов. Правда это исправляет проблему только для нового решения через gap
  • Доработал storybook компонента для более удобной работы с Flex

Release notes

Copy link
Contributor

github-actions bot commented Jan 30, 2025

size-limit report 📦

Path Size
JS 394.44 KB (-0.01% 🔽)
JS (gzip) 119.55 KB (-0.01% 🔽)
JS (brotli) 98.48 KB (+0.12% 🔺)
JS import Div (tree shaking) 1.56 KB (0%)
CSS 347.49 KB (+0.04% 🔺)
CSS (gzip) 43.02 KB (+0.06% 🔺)
CSS (brotli) 34.28 KB (-0.05% 🔽)

Copy link

codesandbox-ci bot commented Jan 30, 2025

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.

Copy link
Contributor

github-actions bot commented Jan 30, 2025

e2e tests

Playwright Report

Copy link
Contributor

github-actions bot commented Jan 30, 2025

👀 Docs deployed

Commit d96c39f

Copy link

codecov bot commented Jan 30, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.51%. Comparing base (750edd0) to head (d96c39f).
Report is 10 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #8212      +/-   ##
==========================================
+ Coverage   95.49%   95.51%   +0.01%     
==========================================
  Files         403      402       -1     
  Lines       11503    11522      +19     
  Branches     3812     3828      +16     
==========================================
+ Hits        10985    11005      +20     
+ Misses        518      517       -1     
Flag Coverage Δ
unittests 95.51% <100.00%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@EldarMuhamethanov EldarMuhamethanov marked this pull request as ready for review January 30, 2025 10:56
Copy link
Contributor

@andrey-medvedev-vk andrey-medvedev-vk left a comment

Choose a reason for hiding this comment

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

👏 👏 👏

SevereCloud
SevereCloud previously approved these changes Jan 30, 2025
@@ -1,5 +1,8 @@
Базовый компонент для позиционирования элементов. Построен на базе `flex layout`.

> ⚠️ Важно: в версиях браузеров `Chrome >= 87`, `Edge >= 87`, `Safari >= 14.1`, `Firefox >= 66`, `Opera >= 73` при использовании
> свойства `gap` используется нативное CSS свойство `gap`. В версиях браузеров ниже используется `fallback` логика с использование свойства `margin`.
Copy link
Contributor

Choose a reason for hiding this comment

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

тут нужно оставить рекомендацию, что если пользователь хочет, чтобы при фолбеке всё было норм, избегать использования React.Fragment, а то сейчас пользователю ничего не говорит факт переключения на фолбек

нужно представить себя пользователем бибилотеки, который ничего не знает о внутренней кухне и читает документацию

Copy link
Contributor

Choose a reason for hiding this comment

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

про React.Fragment это в частности, если есть что-то ещё, то про это тоже стоит упомянуть

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Добавил инфу про React.Fragment, кейс с вложенными Flex у нас покрыт для фолбэка, не стал его писать, чтобы не пугать пользователя. Больше примеров пока не придумал, если будут появляться, то обновим

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: 👀 In Review
Development

Successfully merging this pull request may close these issues.

4 participants