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

fix: (menu & menuitem): vertical menu theming updates #934

Merged
merged 49 commits into from
Mar 5, 2019

Conversation

jaanus03
Copy link
Contributor

The aim of this this PR is to update vertical menu styles to make it look great again.

Current vertical menu styles:
screenshot 2019-02-20 at 14 30 13
screenshot 2019-02-20 at 14 38 26
screenshot 2019-02-20 at 14 38 42

New vertical menu styles:
screenshot 2019-02-20 at 14 43 16
screenshot 2019-02-20 at 14 43 04
screenshot 2019-02-20 at 14 42 46

@jaanus03 jaanus03 added 🚧 WIP 🧰 fix Introduces fix for broken behavior. 💅 styling issue labels Feb 20, 2019
@jaanus03 jaanus03 changed the title fix (menu & menuitem): vertical menu theming updates fix: (menu & menuitem): vertical menu theming updates Feb 20, 2019
@jaanus03 jaanus03 force-pushed the jaanusp/vertical-menu-updates-453393 branch from d075365 to 75360c4 Compare February 20, 2019 13:14
@codecov
Copy link

codecov bot commented Feb 20, 2019

Codecov Report

Merging #934 into master will decrease coverage by 0.05%.
The diff coverage is 20%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #934      +/-   ##
==========================================
- Coverage    81.5%   81.45%   -0.06%     
==========================================
  Files         673      673              
  Lines        8700     8708       +8     
  Branches     1541     1484      -57     
==========================================
+ Hits         7091     7093       +2     
- Misses       1594     1600       +6     
  Partials       15       15
Impacted Files Coverage Δ
.../src/themes/teams/components/Menu/menuVariables.ts 66.66% <ø> (ø) ⬆️
packages/react/src/components/Menu/Menu.tsx 84.9% <ø> (ø) ⬆️
packages/react/src/components/Menu/MenuDivider.tsx 100% <ø> (ø) ⬆️
...kages/react/src/themes/teams-dark/siteVariables.ts 100% <ø> (ø) ⬆️
.../themes/teams/components/Menu/menuDividerStyles.ts 18.18% <0%> (-4.05%) ⬇️
...ms-high-contrast/components/Menu/menuItemStyles.ts 11.76% <0%> (-4.91%) ⬇️
...act/src/themes/teams/components/Menu/menuStyles.ts 13.04% <0%> (-4.35%) ⬇️
...src/themes/teams/components/Menu/menuItemStyles.ts 8.27% <0%> (ø) ⬆️
...themes/teams-dark/components/Menu/menuVariables.ts 100% <100%> (+50%) ⬆️
...ams-high-contrast/components/Menu/menuVariables.ts 100% <100%> (+50%) ⬆️
... and 1 more

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 12d5e4a...3a4c45b. Read the comment docs.

Copy link
Collaborator

@bmdalex bmdalex left a comment

Choose a reason for hiding this comment

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

nice changes, some comments to address

@jaanus03 jaanus03 force-pushed the jaanusp/vertical-menu-updates-453393 branch from 75360c4 to 9575963 Compare February 21, 2019 14:40
@bmdalex
Copy link
Collaborator

bmdalex commented Feb 22, 2019

@jaanus03

I found some time to take the branch and test it. It's true that the Dark and HC themes are much better, but we have a lot of regression on the general theme..

1. wrong fonts for regular theme (master vs your branch):

screenshot 2019-02-22 at 13 19 24

2. underlined menus styles broken:

screen recording 2019-02-22 at 13 23 27

3. primary underlined menus styles COMPLETELY broken:

screen recording 2019-02-22 at 13 35 11

4. iconOnly menus styles broken:

screen recording 2019-02-22 at 13 25 37

5. vertical pointing menus focused state broken:

screen recording 2019-02-22 at 13 31 43

6. focus outline broken for horizontal (no vertical prop) menus:

screen recording 2019-02-22 at 12 52 27

7. even in HC this TabList menu should have other colors I think:

screen recording 2019-02-22 at 13 13 41

@codepretty could you help validating some of these, I know you we're keeping a close eye on menus

@layershifter
Copy link
Member

I think that issue with fonts comes from #950 and will be fixed by #951.

@jaanus03 jaanus03 force-pushed the jaanusp/vertical-menu-updates-453393 branch from 2dcfef2 to b8db4b9 Compare February 22, 2019 15:05
@codepretty
Copy link
Collaborator

codepretty commented Feb 25, 2019

Looks like the menu of the doc site was changed
image

@levithomason do you know how @jaanus03 can exclude his changes from affecting the left rail menu?


Update: chatted with @levithomason about this and he says it is fine to check in

@codepretty
Copy link
Collaborator

Hover and keyboard focus background colors are supposed to be different
image

@jaanus03 jaanus03 force-pushed the jaanusp/vertical-menu-updates-453393 branch from eeaa3d0 to 26515d5 Compare March 5, 2019 11:41
yarn.lock Outdated Show resolved Hide resolved
CHANGELOG.md Show resolved Hide resolved
@mnajdova mnajdova dismissed bmdalex’s stale review March 5, 2019 14:08

Comments were resolved

@jaanus03 jaanus03 merged commit fa65a5c into master Mar 5, 2019
@delete-merged-branch delete-merged-branch bot deleted the jaanusp/vertical-menu-updates-453393 branch March 5, 2019 14:17
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants