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

fix(menu): iconOnly does not overlap big icons #486

Merged
merged 2 commits into from
Dec 4, 2018
Merged

Conversation

bmdalex
Copy link
Collaborator

@bmdalex bmdalex commented Nov 16, 2018

fix(menu): iconOnly does not overlap big icons

Fixes #409

Before fix:

screenshot 2018-11-16 at 15 03 04

After fix:

screenshot 2018-11-16 at 15 22 25

@bmdalex
Copy link
Collaborator Author

bmdalex commented Nov 16, 2018

@miroslavstastny can you take a look?

@codecov
Copy link

codecov bot commented Nov 16, 2018

Codecov Report

❗ No coverage uploaded for pull request base (master@c3cd60f). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #486   +/-   ##
=========================================
  Coverage          ?   88.17%           
=========================================
  Files             ?       42           
  Lines             ?     1455           
  Branches          ?      187           
=========================================
  Hits              ?     1283           
  Misses            ?      167           
  Partials          ?        5

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 c3cd60f...a90977a. Read the comment docs.

@levithomason levithomason added this to Bugaa92 in Core Team Nov 16, 2018
Copy link
Member

@miroslavstastny miroslavstastny left a comment

Choose a reason for hiding this comment

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

From-keyboard-focus-ring moves the icon:
menuicononlyfocus

@miroslavstastny miroslavstastny added needs author changes Author needs to implement changes before merge and removed 🚀 ready for review labels Nov 19, 2018
@bmdalex
Copy link
Collaborator Author

bmdalex commented Nov 19, 2018

@miroslavstastny
nice catch... damn we need these E2E tests, will try come up with a better fix

@bmdalex bmdalex added 💥 blocked postponed Item is postponed and will be reconsidered in future labels Nov 20, 2018
@bmdalex
Copy link
Collaborator Author

bmdalex commented Nov 20, 2018

Blocked by change of priorities; will resume working in upcoming days

@bmdalex bmdalex force-pushed the fix/menu-icon-only branch 3 times, most recently from e81ac13 to 54400fe Compare November 27, 2018 15:52
@bmdalex
Copy link
Collaborator Author

bmdalex commented Nov 27, 2018

@miroslavstastny pls take another look, issue with key nav focus outline is fixed:

icons
icon-sizes

@bmdalex bmdalex added 🚀 ready for review and removed 💥 blocked needs author changes Author needs to implement changes before merge postponed Item is postponed and will be reconsidered in future labels Nov 27, 2018
Copy link
Contributor

@alinais alinais left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@bmdalex bmdalex merged commit 0d969eb into master Dec 4, 2018
@bmdalex bmdalex deleted the fix/menu-icon-only branch December 4, 2018 16:56
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
No open projects
Core Team
  
Bugaa92
Development

Successfully merging this pull request may close these issues.

Larger MenuItem icons overlap when using iconOnly prop
4 participants