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

fix(Popup): styles for dark and HC Teams themes #1121

Merged
merged 7 commits into from
Mar 29, 2019
Merged

fix(Popup): styles for dark and HC Teams themes #1121

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
e80333a
fix styles for dark and hc themes
kuzhelov-ms Mar 27, 2019
828d275
Merge branch 'master' into fix/popup-styles-for-dark-and-hc
kuzhelov Mar 27, 2019
d955bdd
Merge branch 'master' into fix/popup-styles-for-dark-and-hc
kuzhelov-ms Mar 29, 2019
a3d0983
generalise text alignment
kuzhelov-ms Mar 29, 2019
aa5f7e5
update changelog
kuzhelov-ms Mar 29, 2019
e213705
update changelog
kuzhelov-ms Mar 29, 2019
f4d9fe4
Merge branch 'master' into fix/popup-styles-for-dark-and-hc
kuzhelov Mar 29, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions packages/react/src/themes/teams-dark/componentVariables.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,3 +8,6 @@ export { default as RadioGroupItem } from './components/RadioGroup/radioGroupIte
export { default as Text } from './components/Text/textVariables'
export { default as TreeTitle } from './components/Tree/treeTitleVariables'
export { default as Menu } from './components/Menu/menuVariables'

export { default as Popup } from './components/Popup/popupVariables'
export { default as PopupContent } from './components/Popup/popupContentVariables'
5 changes: 5 additions & 0 deletions packages/react/src/themes/teams-dark/components/Popup/popupContentVariables.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
import { PopupContentVariables } from '../../../teams/components/Popup/popupContentVariables'

export default (siteVars: any): Partial<PopupContentVariables> => ({
borderColor: siteVars.colors.black,
})
5 changes: 5 additions & 0 deletions packages/react/src/themes/teams-dark/components/Popup/popupVariables.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
import { PopupVariables } from '../../../teams/components/Popup/popupVariables'

export default (siteVars: any): Partial<PopupVariables> => ({
color: siteVars.colors.white,
})
1 change: 1 addition & 0 deletions packages/react/src/themes/teams-high-contrast/componentVariables.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,3 +9,4 @@ export { default as Menu } from './components/Menu/menuVariables'
export { default as RadioGroupItem } from './components/RadioGroup/radioGroupItemVariables'
export { default as Text } from './components/Text/textVariables'
export { default as TreeTitle } from './components/Tree/treeTitleVariables'
export { default as Popup } from './components/Popup/popupVariables'
5 changes: 5 additions & 0 deletions packages/react/src/themes/teams-high-contrast/components/Popup/popupVariables.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
import { PopupVariables } from '../../../teams/components/Popup/popupVariables'

export default (siteVars: any): Partial<PopupVariables> => ({
color: siteVars.colors.white,
})
5 changes: 2 additions & 3 deletions packages/react/src/themes/teams/components/Popup/popupContentStyles.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,11 @@ import { PopupContentProps } from '../../../../components/Popup/PopupContent'
import { PopupContentVariables } from './popupContentVariables'

const popupContentStyles: ComponentSlotStylesInput<PopupContentProps, PopupContentVariables> = {
root: ({ props, variables }): ICSSInJSStyle => {
const { backgroundColor, borderColor, padding } = variables
root: ({ variables }): ICSSInJSStyle => {
const { borderColor, padding } = variables

return {
display: 'block',
backgroundColor,
padding,
border: `1px solid ${borderColor}`,
borderRadius: pxToRem(3),
Expand Down
2 changes: 0 additions & 2 deletions packages/react/src/themes/teams/components/Popup/popupContentVariables.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,12 @@ import { pxToRem } from '../../../../lib'
export interface PopupContentVariables {
[key: string]: string | number

backgroundColor: string
borderColor: string
padding: string
}

export default (siteVars: any): PopupContentVariables => {
return {
backgroundColor: siteVars.colors.white,
borderColor: siteVars.gray06,
padding: `${pxToRem(10)} ${pxToRem(14)}`,
}
Expand Down
2 changes: 2 additions & 0 deletions packages/react/src/themes/teams/components/Popup/popupStyles.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ const popupStyles: ComponentSlotStylesInput<PopupProps, PopupVariables> = {
popup: ({ variables }): ICSSInJSStyle => ({
zIndex: variables.zIndex,
position: 'absolute',
color: variables.contentColor,
background: variables.contentBackgroundColor,
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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:

image

However, what client really wants here is proper basic styles applied even if plain content is provided to the slot:

image

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)

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure, will take as a follow-up

}),
}

Expand Down
8 changes: 7 additions & 1 deletion packages/react/src/themes/teams/components/Popup/popupVariables.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,12 @@ export interface PopupVariables {
[key: string]: string | number

zIndex: number
contentColor: string
contentBackgroundColor: string
}

export default (siteVars: any): PopupVariables => ({ zIndex: 1000 })
export default (siteVars: any): PopupVariables => ({
zIndex: 1000,
Copy link
Collaborator

Choose a reason for hiding this comment

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

how did we come up with this value?

Copy link
Contributor Author

@kuzhelov kuzhelov Mar 29, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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

contentColor: siteVars.bodyColor,
contentBackgroundColor: siteVars.bodyBackground,
})