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

fix(Tree): fix focus styles #1912

Merged
merged 14 commits into from
Sep 13, 2019
Merged

fix(Tree): fix focus styles #1912

merged 14 commits into from
Sep 13, 2019

Conversation

silviuaavram
Copy link
Collaborator

@silviuaavram silviuaavram commented Sep 10, 2019

Fixes #1620

Adds screener tests for 4 cases:

  1. leaf focus with mouse
  2. leaf focus with kb
  3. non-leaf(subtree item) focus with mouse
  4. non-leaf(subtree item) focus with keyboard

@codecov
Copy link

codecov bot commented Sep 10, 2019

Codecov Report

Merging #1912 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #1912   +/-   ##
======================================
  Coverage    70.5%   70.5%           
======================================
  Files         884     884           
  Lines        7794    7794           
  Branches     2278    2278           
======================================
  Hits         5495    5495           
  Misses       2289    2289           
  Partials       10      10
Impacted Files Coverage Δ
...src/themes/teams/components/Tree/treeItemStyles.ts 50% <ø> (ø) ⬆️
...nts/HierarchicalTree/hierarchicalTreeItemStyles.ts 50% <ø> (ø) ⬆️

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 51f1573...56c7ee4. Read the comment docs.

},
':focus-visible': {
...getBorderFocusStyles({ siteVariables })[':focus-visible'],
},
Copy link
Member

Choose a reason for hiding this comment

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

Previous code does the same as I see

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

before these changes it was not working, we are missing something.

before the removal of isFromKeyboard we had the issue that borderfocus style was applied every time. now, if you check on the stardust docsite, it won't be applied in any case (mouse or keyboard).

these changes fixes it.

Copy link
Member

Choose a reason for hiding this comment

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

Can you please check this thing again?

image

It looks that this change is invalid.

@silviuaavram silviuaavram added accessibility All the Accessibility tasks and bugs should be tagged with this. 🚀 ready for review labels Sep 11, 2019
@layershifter
Copy link
Member

layershifter commented Sep 12, 2019

image

Please do not accept screener tests before merging. Now, it's unclear what was really changed.

},
':focus-visible': {
...getBorderFocusStyles({ siteVariables })[':focus-visible'],
},
Copy link
Member

Choose a reason for hiding this comment

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

@layershifter
Copy link
Member

Please add CHANGELOG entry.

@vercel vercel bot temporarily deployed to staging September 12, 2019 13:53 Inactive
@silviuaavram silviuaavram merged commit 2a9989e into master Sep 13, 2019
@silviuaavram silviuaavram deleted the fix/tree-focus-styles branch September 13, 2019 08:37
layershifter pushed a commit that referenced this pull request Sep 13, 2019
* fix focus styles

* add static Item to Tree

* add screener tests for focus

* fix selector

* fix the selector better

* removed unnecessary static item

* change selector in screener tests

* move all into a single test

* change the test to allow all kb interactions

* revert unneeded change

* changelog

(cherry picked from commit 2a9989e)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accessibility All the Accessibility tasks and bugs should be tagged with this. 🚀 ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tree item has focus ring when navigating using mouse (Teams Theme)
3 participants