williaster / data-ui

A collection of data-rich UI components 📈

https://williaster.github.io/data-ui/

MIT License

546 stars 69 forks source link

[shared]: fix issue #167 #171

Closed gnijuohz closed 5 years ago

gnijuohz commented 5 years ago

💔 Breaking Changes

🏆 Enhancements

📜 Documentation

🐛 Bug Fix

Fix issue #167, after this fix, a focus event won't show the tooltip anymore.

An alternative fix I tried was to add && event.type !== 'focus' to the condition here https://github.com/williaster/data-ui/blob/master/packages/shared/src/enhancer/WithTooltip.jsx#L71. In that case clicking on a bar for the first time (focusing it) would move the position of the bar. What do you think about this? @williaster

🏠 Internal

codecov[bot] commented 5 years ago

Codecov Report

Merging #171 into master will decrease coverage by 0.43%. The diff coverage is 0%.

@@            Coverage Diff             @@
##           master     #171      +/-   ##
==========================================
- Coverage   80.51%   80.07%   -0.44%     
==========================================
  Files         109      109              
  Lines        2422     2424       +2     
  Branches      568      569       +1     
==========================================
- Hits         1950     1941       -9     
- Misses        291      302      +11     
  Partials      181      181

Impacted Files	Coverage Δ
packages/shared/src/enhancer/WithTooltip.jsx	`46.87% <0%> (-33.13%)`	:arrow_down:

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 a7abd58...dd8ab83. Read the comment docs.

codecov[bot] commented 5 years ago

Codecov Report

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

@@           Coverage Diff           @@
##           master     #171   +/-   ##
=======================================
  Coverage   80.51%   80.51%           
=======================================
  Files         109      109           
  Lines        2422     2422           
  Branches      568      568           
=======================================
  Hits         1950     1950           
  Misses        291      291           
  Partials      181      181

Impacted Files	Coverage Δ
packages/shared/src/enhancer/WithTooltip.jsx	`80% <0%> (ø)`	:arrow_up:

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 a7abd58...b12d02d. Read the comment docs.

williaster commented 5 years ago

@gnijuohz thanks for the PR! I want to pull this and test locally to confirm that accessibility still works. Hopefully can merge in the next couple days.

williaster commented 5 years ago

Yeah sorry, this breaks tab-able tooltips entirely which was introduced as a requirement for accessibility.

However it looks like tab-ing doesn't work at all in firefox either, which is another bug in and of itself 😞 I'm a little confused, maybe we need another approach aside from wrapping in <a> tags 🤔

gnijuohz commented 5 years ago

Hi @williaster you're right. My previous fix prevents tooltips from showing up on focus. My updated fix should no longer break that.

williaster commented 5 years ago

Thanks @gnijuohz ! Pulled again to test and functionally it seems good to me (the movement of the tooltip on click doesn't seem super problematic to me)

gnijuohz commented 5 years ago

Thanks @williaster !

I haven't done much research on focusing elements in svg, what do you think about using tabindex? It seems to have great support from major browsers: https://developer.mozilla.org/en-US/docs/Web/SVG/Attribute/tabindex