fix(Popup): styles for dark and HC Teams themes #1121
Conversation
kuzhelov
requested review from
dvdzkwsk,
layershifter,
levithomason,
miroslavstastny and
mnajdova
as code owners
Codecov Report
@@ Coverage Diff @@
## master #1121 +/- ##
==========================================
+ Coverage 82.36% 82.37% +0.01%
==========================================
Files 727 729 +2
Lines 8674 8679 +5
Branches 1232 1232
==========================================
+ Hits 7144 7149 +5
Misses 1515 1515
Partials 15 15
Continue to review full report at Codecov.
|
| @@ -8,6 +8,8 @@ const popupStyles: ComponentSlotStylesInput<PopupProps, PopupVariables> = { | |||
| popup: ({ variables }): ICSSInJSStyle => ({ | |||
| zIndex: variables.zIndex, | |||
| position: 'absolute', | |||
| color: variables.contentColor, | |||
| background: variables.contentBackgroundColor, | |||
There was a problem hiding this comment.
Is there a reason why we moved the background style from the PopupContent to the Popup? I'd prefer to see them in it's own component styles, rather then in the popup, especially because they are not anyhow dependent on some props from the Popup component.
There was a problem hiding this comment.
yes - the reason is that we want to see the background and foreground colors being applied to the popup's content irregardless of what is rendered as popup content. Previously we have introduced proper styles only for the cases when PopupContent was rendered at the content slot - however, in case if user would render plain content there, there won't be any background/foreground styles applied at all. Thus, the following code
<Popup ... content={<div>Hello from popup content!</div>} />would result in the presentation that was mentioned in the PR's description:
However, what client really wants here is proper basic styles applied even if plain content is provided to the slot:
That's why the variables were moved - note that with that we are not limiting client in overriding styles for the content in case if necessary (so, if content provides its own styles for background and color, then those will be used)
| } | ||
|
|
||
| export default (siteVars: any): PopupVariables => ({ zIndex: 1000 }) | ||
| export default (siteVars: any): PopupVariables => ({ | ||
| zIndex: 1000, |
There was a problem hiding this comment.
how did we come up with this value?
There was a problem hiding this comment.
not sure, agreed to use the one that has worked for us back then :) Low numbers might put Popup design at risk, as there might be some client's components that rely on some elements shown on top of the others, and z-index: 1 + relative positioning is always the thing that does this trick. So, admittedly, for the sake of being on a safer side this value was introduced
| @@ -8,6 +8,8 @@ const popupStyles: ComponentSlotStylesInput<PopupProps, PopupVariables> = { | |||
| popup: ({ variables }): ICSSInJSStyle => ({ | |||
| zIndex: variables.zIndex, | |||
| position: 'absolute', | |||
| color: variables.contentColor, | |||
| background: variables.contentBackgroundColor, | |||
There was a problem hiding this comment.
since now we have relevant styles for Dark and HC themes, can we add a few screenshot tests for these themes for relevant Popup examples?
It'd be great to add some steps too, but that can be in different PR, something like:
- open a Popup by click and take snapshot
- open a Popup by key and take snapshot
- repeat these steps for different variations: inline Popups, Popups with positioning, etc
There was a problem hiding this comment.
sure, will take as a follow-up
There was a problem hiding this comment.
We should find a way how to share styles between ProviderBox and Popup.
After this change, whatever we add to ProviderBox, we need to add it to Popup again.
Moreover, whatever a consumer overrides in ProviderBox, (s)he must override it again for each Popup.
See #1115 as an example of this.
TODO
Fixes #1053.
Provided changes address the problem of Popup styling for Dark and High Contrast Teams themes. In addition, variables that control
Popup's content styling were introduced, so that now any custom popup content will be properly displayed with the right set of foreground and background colors.Now
Dark
High Contrast
Custom popup content (plain div)
Was (barely noticeable black text on dark background)
Now