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

fix(MenuItem): icon only active state in Teams theme #1464

Merged
merged 10 commits into from
Jun 7, 2019

Conversation

mnajdova
Copy link
Contributor

@mnajdova mnajdova commented Jun 7, 2019

Fix #1465

Menu with icon only, didn't show any active state.

Teams default theme

Before:
image
After:
image

Teams dark theme

Before:
image
After:
image

Teams high-contrast theme

Before:
image
After:
image

Question

Not sure whether the icon in hc should be outline/filled. On focus it is filled, so I added the same logic here. I may revert this changes.

@codecov
Copy link

codecov bot commented Jun 7, 2019

Codecov Report

Merging #1464 into master will not change coverage.
The diff coverage is 0%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1464   +/-   ##
=======================================
  Coverage   73.14%   73.14%           
=======================================
  Files         805      805           
  Lines        6073     6073           
  Branches     1775     1794   +19     
=======================================
  Hits         4442     4442           
  Misses       1625     1625           
  Partials        6        6
Impacted Files Coverage Δ
...ams-high-contrast/components/Menu/menuVariables.ts 0% <ø> (ø) ⬆️
...src/themes/teams/components/Menu/menuItemStyles.ts 7.2% <0%> (ø) ⬆️

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 47dca77...36312ba. Read the comment docs.

...(iconOnly && { color: 'inherit' }),
...(active && {
...(iconOnly && {
color: v.iconOnlyColorActive,
Copy link
Contributor

@kuzhelov kuzhelov Jun 7, 2019

Choose a reason for hiding this comment

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

just to be sure - could you, please, suggest where original inherit value had resulted in suboptimal styles?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In team's theme the iconOnly Menu should always have the brand color for the active items. The inherit color is black in default theme.

@@ -2,23 +2,27 @@ import { Menu } from '@stardust-ui/react'

interface StepsOptions {
vertical?: boolean
startIdx?: number
endIdx?: number
Copy link
Member

Choose a reason for hiding this comment

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

startItem & endItem?

@kuzhelov
Copy link
Contributor

kuzhelov commented Jun 7, 2019

@codepretty, could you, please, suggest what is prescribed by Teams design spec for the case @mnajdova had mentioned in the description? Thank you!

Not sure whether the icon in HC should be outline/filled. On focus it is filled, so I added the same logic here. I may revert this changes.

@mnajdova mnajdova merged commit 6fb51c8 into master Jun 7, 2019
@delete-merged-branch delete-merged-branch bot deleted the fix/menu-item-icon-only-active-color branch June 7, 2019 17:04
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Teams theme: <MenuItem iconOnly active> not highlighted
3 participants