Prevent native context menu from being invoked on webviews #1736
Conversation
![codeclimate[bot]](https://avatars.githubusercontent.com/in/9403?s=60&v=4){kind=small}
| // eslint-disable-next-line sonarjs/cognitive-complexity | ||
| export const Plots = ({ state }: { state: PlotsWebviewState }) => { | ||
| const PlotsContent = ({ state }: PlotsProps) => { |
There was a problem hiding this comment.
Function PlotsContent has a Cognitive Complexity of 8 (exceeds 5 allowed). Consider refactoring.
| @@ -12,7 +12,7 @@ type CSSVariables = DetailedHTMLProps< | |||
| HTMLDivElement | |||
| > | |||
|
|
|||
| export const Theme: React.FC = ({ children }) => { | |||
| export const useThemeVariables = () => { | |||
There was a problem hiding this comment.
Function useThemeVariables has 34 lines of code (exceeds 30 allowed). Consider refactoring.
| export const Plots = ({ state }: PlotsProps) => { | ||
| const variables = useThemeVariables() | ||
|
|
||
| return ( | ||
| <div | ||
| style={variables} | ||
| onContextMenu={e => { | ||
| e.preventDefault() | ||
| }} | ||
| > | ||
| <PlotsContent state={state} /> | ||
| </div> |
There was a problem hiding this comment.
This container refactor makes it easier to ensure all states are wrapped with click-prevention even with the early-return logic we had before. It could probably use a larger refactor, but that's not the focus of this PR.
| export const Theme: React.FC = ({ children }) => { | ||
| const variables = useThemeVariables() |
There was a problem hiding this comment.
I kept Theme for backward-compatibility, but it'd probably be best to use the hook everywhere in the future.
There was a problem hiding this comment.
I don't think backward-compatibility is the right terminology here. If we are going to use the hook can you please follow up and remove this component. Thank you.
|
Closes #1723 |
| export const Theme: React.FC = ({ children }) => { | ||
| const variables = useThemeVariables() |
There was a problem hiding this comment.
I don't think backward-compatibility is the right terminology here. If we are going to use the hook can you please follow up and remove this component. Thank you.
…ontext-menu-on-webviews
| // eslint-disable-next-line sonarjs/cognitive-complexity | ||
| export const Plots = ({ state }: { state: PlotsWebviewState }) => { | ||
| const PlotsContent = ({ state }: PlotsProps) => { |
There was a problem hiding this comment.
Function PlotsContent has a Cognitive Complexity of 10 (exceeds 5 allowed). Consider refactoring.
|
Code Climate has analyzed commit bf2312a and detected 2 issues on this pull request. Here's the issue category breakdown:
The test coverage on the diff in this pull request is 100.0% (85% is the threshold). This pull request will bring the total coverage in the repository to 96.7% (0.0% change). View more on Code Climate. |
Demo in VSCode:
suppressed-contextmenu-vscode-demo.mp4
Demo in Storybook (proving it works in no-data states):
suppressed-contextmenu-storybook-demo.mp4
This PR suppresses native context menu in both Experiments Table and Plots webviews by adding a wrapper
divwith the proponContextmenu={(e)=>{e.preventDefault()}}as the outermost component for both webviews.In order to keep unnecessary wrappers to a minimum, I also refactored the
Themecomponent's logic out into a hook which can be applied to normaldivs so that we can use the same wrapperdivfor both purposes.