Skip to content

Fix IconButton styles for hover, focus, and pressed. #4498

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.

Sign up for GitHub

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

Merged
jordandrako merged 6 commits into from
Apr 10, 2018
Merged

Fix IconButton styles for hover, focus, and pressed. #4498

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
94d986f
Fix IconButton styles for hover, focus, and pressed.
Apr 9, 2018
b4ef0c4
Add color back.
Apr 9, 2018
72d0b4f
Update snapshots, add change file.
Apr 9, 2018
5769cc3
Remove focus styles
Apr 9, 2018
e7422c3
update snapshots
Apr 9, 2018
adeb0b9
Switch to semanticColors
Apr 9, 2018
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
11 changes: 11 additions & 0 deletions .../changes/office-ui-fabric-react/hotfix-IconButton-states-background_2018-04-09-21-14.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
{
"changes": [
{
"packageName": "office-ui-fabric-react",
"comment": "Fix IconButton state styles",
"type": "patch"
}
],
"packageName": "office-ui-fabric-react",
"email": "jordanjanzen@jordanjanzen.com"
}
5 changes: 5 additions & 0 deletions ...s/office-ui-fabric-react/src/components/Breadcrumb/__snapshots__/Breadcrumb.test.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -242,9 +242,14 @@ exports[`Breadcrumb renders breadcumb correctly 2`] = `
top: -2px;
}
&:hover {
background-color: #f4f4f4;
color: #004578;
}
&:focus {
border: 1px solid #666666;
}
&:active {
background-color: #eaeaea;
color: #0078d4;
}
data-is-focusable={true}
Expand Down
10 changes: 8 additions & 2 deletions packages/office-ui-fabric-react/src/components/Button/IconButton/IconButton.styles.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,11 +26,17 @@ export const getStyles = memoizeFunction((
},

rootHovered: {
color: theme.palette.themeDarker
color: theme.palette.themeDarker,
backgroundColor: theme.palette.neutralLighter,

@Jahnp Jahnp Apr 9, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Are there semantic color slots we can use for these?

@jordandrako jordandrako Apr 9, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah.
semanticColors.buttonBackground = palette.neutralLighter (the color Ben requested for Hovered state)
semanticColors.buttonBackgroundHovered = palette.neutralLight (the color Ben requested for Pressed state)

@Jahnp Jahnp Apr 9, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Awesome--if that's the case, I'd think we should use those instead so IconButton can pick up themes moving forward. @betrue-final-final @phkuo for FYI

@jordandrako jordandrako Apr 9, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Alright, I'll change all those over. Is there a semanticColor for palette.themePrimary or leave those alone in this case?

@jordandrako jordandrako Apr 9, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@Jahnp One more review?

@Jahnp Jahnp Apr 9, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Totally fair--duh. I must have missed the background portion. There doesn't seem to a perfect match--menuIcon seems like the next closest, but it feels wrong to apply "menu" to something that isn't in a menu.

@phkuo -- is this a case where we'd want to consider a new slot, or is it acceptable to stretch the usage of an existing slot, like using menuIcon for the foreground of an icon button?

@jordandrako jordandrako Apr 9, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

There are three possible ones I'd use: menuIcon, menuHeader, or link.
@mikewheaton Does palette.themePrimary need to be replaced in scenarios like this? Doesn't it get its value from the theme HOC too?

@phkuo phkuo Apr 9, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@jordandrako both palette and semantic colors theme. If uncustomized, semantic colors will pick up an appropriate palette color.

Don't try to "stretch" the meaning of a semantic slot too far, we shouldn't use menu slots for buttons. We do need new color slots for "text buttons", buttons that aren't outlined.

@jordandrako jordandrako Apr 10, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Alright, then I think we should leave the themePrimary uses alone since they're applied to the IconButton's foreground color. @Jahnp cool?

@Jahnp Jahnp Apr 10, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ok, as long as having a mix of semantic & palette colors is fine, that works for me.

},

rootFocused: {
border: `1px solid ${theme.palette.neutralSecondary}`,
},

rootPressed: {
color: theme.palette.themePrimary
color: theme.palette.themePrimary,
backgroundColor: theme.palette.neutralLight,
},

rootExpanded: {
Expand Down
5 changes: 5 additions & 0 deletions packages/office-ui-fabric-react/src/components/Button/__snapshots__/Button.test.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -571,9 +571,14 @@ exports[`Button renders IconButton correctly 1`] = `
top: -2px;
}
&:hover {
background-color: #f4f4f4;
color: #004578;
}
&:focus {
border: 1px solid #666666;
}
&:active {
background-color: #eaeaea;
color: #0078d4;
}
data-is-focusable={true}
Expand Down
3 changes: 3 additions & 0 deletions packages/office-ui-fabric-react/src/components/ComboBox/__snapshots__/ComboBox.test.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,6 +224,9 @@ exports[`ComboBox Renders ComboBox correctly 1`] = `
color: #212121;
cursor: pointer;
}
&:focus {
border: 1px solid #666666;
}
&:active {
background-color: #eaeaea;
color: #212121;
Expand Down
6 changes: 6 additions & 0 deletions ...s/office-ui-fabric-react/src/components/SpinButton/__snapshots__/SpinButton.test.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,6 +205,9 @@ exports[`SpinButton renders SpinButton correctly 1`] = `
background-color: #eaeaea;
color: #000000;
}
&:focus {
border: 1px solid #666666;
}
&:active {
background-color: #c8c8c8;
color: #212121;
Expand Down Expand Up @@ -318,6 +321,9 @@ exports[`SpinButton renders SpinButton correctly 1`] = `
background-color: #eaeaea;
color: #000000;
}
&:focus {
border: 1px solid #666666;
}
&:active {
background-color: #c8c8c8;
color: #212121;
Expand Down