Skip to content
This repository was archived by the owner on Mar 4, 2020. It is now read-only.

fix(Popup): fix styling with pointer when it is wrapped by FocusZones#1492

Merged
layershifter merged 3 commits intomasterfrom
fix/popup-pointer-fz
Jun 14, 2019
Merged

fix(Popup): fix styling with pointer when it is wrapped by FocusZones#1492
layershifter merged 3 commits intomasterfrom
fix/popup-pointer-fz

Conversation

@layershifter
Copy link
Member

@layershifter layershifter commented Jun 12, 2019

Fixes #1433.


Problem

When we use behaviors for Popup that have FocusZones, it results in new div and changed DOM structure, which causes broken styles.

Solution

Provide wrapped prop to PopupContent, so we will know if it is wrapped by FocusZones and provide correct styles. This solution is short-term and aims only to fix bug - as any refactoring that will make it simpler and will allow to avoid wrapped prop will contain breaking changes, which we want to allow for now.

I decided to prefer prop instead of passing down the variables because we don't have a proper merging of them.

@layershifter layershifter force-pushed the fix/popup-pointer-fz branch from 547bb2c to 0fbb5c2 Compare June 14, 2019 09:07
@layershifter layershifter force-pushed the fix/popup-pointer-fz branch from 0fbb5c2 to 7117c60 Compare June 14, 2019 09:15
@layershifter layershifter changed the title Fix/popup pointer fz fix(Popup): fix styling with pointer when it is wrapped by FocusZones Jun 14, 2019
@layershifter layershifter marked this pull request as ready for review June 14, 2019 09:20
@layershifter layershifter added 🚀 ready for review 🧰 fix Introduces fix for broken behavior. labels Jun 14, 2019
@codecov
Copy link

codecov bot commented Jun 14, 2019

Codecov Report

Merging #1492 into master will decrease coverage by <.01%.
The diff coverage is 50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1492      +/-   ##
==========================================
- Coverage   73.44%   73.43%   -0.01%     
==========================================
  Files         822      822              
  Lines        6190     6192       +2     
  Branches     1795     1797       +2     
==========================================
+ Hits         4546     4547       +1     
- Misses       1639     1640       +1     
  Partials        5        5
Impacted Files Coverage Δ
...ckages/react/src/components/Popup/PopupContent.tsx 90.9% <ø> (ø) ⬆️
...hemes/teams/components/Popup/popupContentStyles.ts 16.66% <0%> (-3.34%) ⬇️
packages/react/src/components/Popup/Popup.tsx 68.15% <100%> (+0.2%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1041c9a...8e9963e. Read the comment docs.

Copy link
Contributor

@mnajdova mnajdova left a comment

Choose a reason for hiding this comment

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

Don't have better idea for now. We may actually add the wrapper prop, as it is in the menu, but we really need just a boolean here for now, so as a short term fix, I am okay with this.

CHANGELOG.md Outdated

### Fixes
- Fix prop types of `Tooltip` component @kuzhelov ([#1499](https://github.com/stardust-ui/react/pull/1499))
- Fix `Popup` styling with `pointer` when it is wrapped by FocusZones @layershifter ([#1492](https://github.com/stardust-ui/react/pull/1492))
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: lets use code formatting for FocusZones

pointerRef?: React.Ref<Element>

/**
* Indicates that PopupContent is wrapped with FocusZone. Do not use it, it used only for internal implementation and
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: lets use @deprecated pragma to make proper accent on that - as this pragma is commonly supported and visualised (at least) by IDEs

Copy link
Member Author

Choose a reason for hiding this comment

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

Will do, but you accidentally tagged someone 😸

display: 'block',

...(p.unstable_wrapped && {
backgroundColor: 'inherit',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wouldn't it be enough to add the styles all the time? without the condition of p.unstable_wrapped? I mean maybe we can avoid this prop altogether... Or will adding these styles cause different issue?

Copy link
Member Author

Choose a reason for hiding this comment

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

If we will always add these styles we will inherit backgroundColor from body when there are no wrapper...

@layershifter layershifter merged commit bedc0a1 into master Jun 14, 2019
@delete-merged-branch delete-merged-branch bot deleted the fix/popup-pointer-fz branch June 14, 2019 15:35
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

🧰 fix Introduces fix for broken behavior. 🚀 ready for review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Teams theme: Popup pointing broken styles with focus trap

5 participants