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

feat(ImageBasePositionedComponent): add subcomponent to positioning component in Image #7166

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

Conversation

EldarMuhamethanov
Copy link
Contributor

@EldarMuhamethanov EldarMuhamethanov commented Jul 10, 2024


  • Unit-тесты
  • Документация фичи

Описание

Нужно добавить сабкомпонент для компонента Image для позиционированирования компонентов внутри Image.

Изменения

  • Добавил сабкомпонент ImageBaseFloatElement для абсолютного позиционирования компонентов в компоненте Image.
  • Добавил тесты для компонента
  • Добавил стори для компонента
  • Добавил использование компонента в документацию

Release notes

Новые компоненты

  • Image: Добавлен сабкомпонент Image.FloatElement для позиционирования компонента относительно картинки

…omponent in Image

- Добавил сабкомпонент ImageBasePositionedComponent для абсолютного позиционирования компонентов в компоненте Image.
- Добавил тесты для компонента
- Добавил стори для компонента
- Добавил использование компонента в документацию
@EldarMuhamethanov EldarMuhamethanov requested a review from a team as a code owner July 10, 2024 09:36
Copy link
Contributor

github-actions bot commented Jul 10, 2024

size-limit report 📦

Path Size
JS 390.09 KB (+0.9% 🔺)
JS (gzip) 117.65 KB (+0.66% 🔺)
JS (brotli) 96.59 KB (+0.62% 🔺)
JS import Div (tree shaking) 1.46 KB (0%)
CSS 337.95 KB (+1.31% 🔺)
CSS (gzip) 42.19 KB (+0.93% 🔺)
CSS (brotli) 33.39 KB (+0.99% 🔺)

Copy link

codesandbox-ci bot commented Jul 10, 2024

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 Jul 10, 2024

e2e tests

Playwright Report

Copy link
Contributor

github-actions bot commented Jul 10, 2024

👀 Docs deployed

Commit 537bc0e

Copy link

codecov bot commented Jul 10, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.17%. Comparing base (763daf7) to head (537bc0e).
Report is 84 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #7166      +/-   ##
==========================================
+ Coverage   95.14%   95.17%   +0.02%     
==========================================
  Files         384      386       +2     
  Lines       11357    11426      +69     
  Branches     3727     3746      +19     
==========================================
+ Hits        10806    10875      +69     
  Misses        551      551              
Flag Coverage Δ
unittests 95.17% <100.00%> (+0.02%) ⬆️

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.

Copy link
Contributor

@mendrew mendrew left a comment

Choose a reason for hiding this comment

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

Выглядит здорово.

Я так понимаю, что он сразу сделан с возможностью использовать несколько Image.PositionedComponent. 👍

Оставил пару комментариев.

Copy link
Contributor

@mendrew mendrew left a comment

Choose a reason for hiding this comment

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

Шикарно 🔥

mendrew
mendrew previously approved these changes Jul 17, 2024
Copy link
Contributor

@mendrew mendrew left a comment

Choose a reason for hiding this comment

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

👍

Comment on lines 125 to 143
React.useEffect(
function updateHiddenSubscribe() {
const wrapper = (containerRef && containerRef.current) || imageWrapperRef.current;
if (wrapper && visibility === 'on-image-hover') {
const onMouseOver = () => setHidden(false);
const onMouseOut = () => setHidden(true);

wrapper.addEventListener('mouseover', onMouseOver);
wrapper.addEventListener('mouseout', onMouseOut);

return () => {
wrapper.removeEventListener('mouseover', onMouseOver);
wrapper.removeEventListener('mouseout', onMouseOut);
};
}
return;
},
[visibility, imageWrapperRef, containerRef],
);
Copy link
Contributor

@inomdzhon inomdzhon Jul 19, 2024

Choose a reason for hiding this comment

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

Не думал по поводу такой реализации?

<ImageBase src="">
  <ImageBase.Overlay
    // можно без disableInteractive если хочется клик на весь блок
    disableInteractive
  >
    <ImageBase.OverlayFloatElement position="top-start">
      <Button>Кнопка</Button>
    </ImageBase.OverlayFloatElement>
  </ImageBase.Overlay>
</ImageBase>
  1. разрешит непонятку по поводу совместного использования с ImageBase.Overlay, в целом идеологически будет корректней;
  2. не нужно будет вещать события mouse, т.к. ImageBase.Overlay уже умеет в это – параметры visibility и containerRef можно будет удалить

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Точно ли будет удобно пользоваться такой API. Для каждого элемента, надо будет создавать OverlayFloatElement обернутый в Overlay

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@inomdzhon Попробовал реализовать через Overlay и понял, что будут проблемы, когда нужно будет спозиционировать несколько элементов на картинке, потому что overlay растягивается на весь размер картинки. И получается, что последний отрендеренный элемент перекроет другие и не появится при наведении на картинку

Copy link
Contributor

@inomdzhon inomdzhon Aug 27, 2024

Choose a reason for hiding this comment

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

имеешь ввиду про вот такой кейс?

<ImageBase src="">
  <ImageBase.Overlay
    // можно без disableInteractive если хочется клик на весь блок
    disableInteractive
  >
    <ImageBase.OverlayFloatElement position="top-start">
      <Button>👍</Button>
    </ImageBase.OverlayFloatElement>
    
    <ImageBase.OverlayFloatElement position="top-start">
      <Button>⚙️</Button>
    </ImageBase.OverlayFloatElement>
  </ImageBase.Overlay>
</ImageBase>

если да, то это скорее вот так должно быть свёрстано

<ImageBase src="">
  <ImageBase.Overlay
    // можно без disableInteractive если хочется клик на весь блок
    disableInteractive
  >
    <ImageBase.OverlayFloatElement position="top-start">
      <Flex>
        <Flex.Item><Button>👍</Button></Flex.Item>
        <Flex.Item><Button>⚙️</Button></Flex.Item>
      </Flex>
    </ImageBase.OverlayFloatElement>
  </ImageBase.Overlay>
</ImageBase>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

да, так будет работать, но не во всех кейсах. Если понадобится сделать так, чтобы один элемент был виден всегда, а другой только при наведении на картинку. Тогда по идее верстка будет следующая:

<ImageBase src="">
  <ImageBase.Overlay
    // можно без disableInteractive если хочется клик на весь блок
    disableInteractive
    visibility="on-hover"
  >
    <ImageBase.OverlayFloatElement position="top-start">
      <Flex.Item><Button>👍</Button></Flex.Item>
    </ImageBase.OverlayFloatElement>
  </ImageBase.Overlay>
  
  <ImageBase.Overlay
    // можно без disableInteractive если хочется клик на весь блок
    disableInteractive
    visibility="always"
  >
    <ImageBase.OverlayFloatElement position="bottom-start">
      <Flex.Item><Button>⚙️</Button></Flex.Item>
    </ImageBase.OverlayFloatElement>
  </ImageBase.Overlay>
</ImageBase>

При этом второй элемент перекроет первый и первый не появится при наведении

Copy link
Contributor

Choose a reason for hiding this comment

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

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

@MrZillaGold у вас есть такой кейс?

Кстати, к этой задаче нужно подключить дизайн (@Zaycevq или @qurle) – на уровне дизайн-системы нужно зафиксировать поведение

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@inomdzhon Обсудили с дизайном, могут быть кейсы с несколькими элементами

@vkcom-publisher vkcom-publisher added pr-needs-work Автоматизация: PR автоматически закроется через 14 дней при отсутствии активности and removed pr-needs-work Автоматизация: PR автоматически закроется через 14 дней при отсутствии активности labels Jul 30, 2024
@vkcom-publisher vkcom-publisher added the pr-needs-work Автоматизация: PR автоматически закроется через 14 дней при отсутствии активности label Aug 7, 2024
@vkcom-publisher vkcom-publisher removed the pr-needs-work Автоматизация: PR автоматически закроется через 14 дней при отсутствии активности label Aug 8, 2024
@vkcom-publisher vkcom-publisher added pr-needs-work Автоматизация: PR автоматически закроется через 14 дней при отсутствии активности and removed pr-needs-work Автоматизация: PR автоматически закроется через 14 дней при отсутствии активности labels Aug 20, 2024
Comment on lines +78 to +81
/**
* Позиция компонента относительно родителя
*/
position: FloatElementPlacement | FloatElementPosition;
Copy link
Contributor

Choose a reason for hiding this comment

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

что на счёт вместо FloatElementPosition использовать синтаксис <x> <y> + вынести это в отдельный параметрм offset? Выбираешь где будет элемент, а если нужно, добавляешь смещение

<ImageBaseFloatElement position="top" offset="5% 5%">
  <Icon24Add />
</ImageBaseFloatElement>

<ImageBaseFloatElement position="middle-start" offset="0 10px">
  <Icon24Add />
</ImageBaseFloatElement>

или как в lib/floating/useFloatingMiddlewaresBootstrap/index.ts сделать два отдельных параметра offsetByMainAxis и offsetByCrossAxis

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@vkcom-publisher vkcom-publisher added pr-needs-work Автоматизация: PR автоматически закроется через 14 дней при отсутствии активности and removed pr-needs-work Автоматизация: PR автоматически закроется через 14 дней при отсутствии активности labels Sep 6, 2024
@vkcom-publisher vkcom-publisher added the pr-needs-work Автоматизация: PR автоматически закроется через 14 дней при отсутствии активности label Sep 26, 2024
@vkcom-publisher vkcom-publisher removed the pr-needs-work Автоматизация: PR автоматически закроется через 14 дней при отсутствии активности label Sep 27, 2024
@vkcom-publisher vkcom-publisher added pr-needs-work Автоматизация: PR автоматически закроется через 14 дней при отсутствии активности and removed pr-needs-work Автоматизация: PR автоматически закроется через 14 дней при отсутствии активности labels Oct 4, 2024
@vkcom-publisher vkcom-publisher added pr-needs-work Автоматизация: PR автоматически закроется через 14 дней при отсутствии активности and removed pr-needs-work Автоматизация: PR автоматически закроется через 14 дней при отсутствии активности labels Oct 13, 2024
@vkcom-publisher vkcom-publisher added the pr-needs-work Автоматизация: PR автоматически закроется через 14 дней при отсутствии активности label Oct 21, 2024
@vkcom-publisher vkcom-publisher removed the pr-needs-work Автоматизация: PR автоматически закроется через 14 дней при отсутствии активности label Oct 22, 2024
@vkcom-publisher vkcom-publisher added pr-needs-work Автоматизация: PR автоматически закроется через 14 дней при отсутствии активности and removed pr-needs-work Автоматизация: PR автоматически закроется через 14 дней при отсутствии активности labels Oct 29, 2024
@vkcom-publisher vkcom-publisher added the pr-needs-work Автоматизация: PR автоматически закроется через 14 дней при отсутствии активности label Nov 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr-needs-work Автоматизация: PR автоматически закроется через 14 дней при отсутствии активности
Projects
None yet
5 participants