Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove outline from clicking on SVG chord tracks #2161

Merged
merged 2 commits into from
Jul 26, 2021
Merged

Conversation

cmdcolin
Copy link
Collaborator

Found in #2160

@github-actions github-actions bot added the needs label triage Needs a label to show in changelog (breaking, enhancement, bug, documentation, or internal) label Jul 26, 2021
@cmdcolin cmdcolin requested a review from garrettjstevens July 26, 2021 20:27
@codecov
Copy link

codecov bot commented Jul 26, 2021

Codecov Report

Merging #2161 (865a7b3) into main (ac57d08) will increase coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2161      +/-   ##
==========================================
+ Coverage   61.95%   61.98%   +0.02%     
==========================================
  Files         476      476              
  Lines       22782    22782              
  Branches     5349     5349              
==========================================
+ Hits        14115    14121       +6     
+ Misses       8386     8380       -6     
  Partials      281      281              
Impacted Files Coverage Δ
...BaseChordDisplay/components/RpcRenderedSvgGroup.js 88.23% <100.00%> (ø)
packages/core/util/index.ts 79.78% <0.00%> (ø)
...nts/src/SNPCoverageRenderer/SNPCoverageRenderer.ts 87.65% <0.00%> (+7.40%) ⬆️

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 ac57d08...865a7b3. Read the comment docs.

Copy link
Collaborator

@garrettjstevens garrettjstevens left a comment

Choose a reason for hiding this comment

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

This seems like a good place to add the fix. I think we might want to add focusable: false, too. Also, was it intentional to remove domNode.style.display = 'inline'?

@cmdcolin
Copy link
Collaborator Author

Also, was it intentional to remove domNode.style.display = 'inline'?

Yea, I don't believe that display:inline has any bearing in the svg

This seems like a good place to add the fix. I think we might want to add focusable: false, too

hmmm, adding it e.g. domNode.focusable=false does not show up in the DOM. not sure if that one is needed?

@cmdcolin cmdcolin added bug Something isn't working and removed needs label triage Needs a label to show in changelog (breaking, enhancement, bug, documentation, or internal) labels Jul 26, 2021
@garrettjstevens
Copy link
Collaborator

Ok, turns out that focusable isn't in the current SVG spec (it was in an old one), so no need to add it.

@cmdcolin cmdcolin merged commit 7d4e500 into main Jul 26, 2021
@cmdcolin cmdcolin deleted the remove_outline_svg branch July 26, 2021 21:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants