Skip to content

Commit

Permalink
Address review comments for call tree transforms
Browse files Browse the repository at this point in the history
  • Loading branch information
gregtatum committed Oct 23, 2020
1 parent dbd9db6 commit 755b7ad
Show file tree
Hide file tree
Showing 7 changed files with 38 additions and 29 deletions.
6 changes: 3 additions & 3 deletions res/css/style.css
Original file line number Diff line number Diff line change
Expand Up @@ -495,9 +495,9 @@ body,
.react-contextmenu-item.react-contextmenu-item--active,
.react-contextmenu-item:hover,
.react-contextmenu-item--selected {
border-color: highlight;
background-color: highlight;
color: highlighttext;
border-color: var(--blue-60);
background-color: var(--blue-60);
color: #fff;
text-decoration: none;
}

Expand Down
8 changes: 4 additions & 4 deletions res/img/svg/merge-icon.svg
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
9 changes: 4 additions & 5 deletions src/components/shared/CallNodeContextMenu.css
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@
margin-inline-end: 8px;
pointer-events: auto;
}

.react-contextmenu-item--selected > .callNodeContextMenuIcon,
.react-contextmenu-item:hover .callNodeContextMenuIcon,
.react-contextmenu-item:active .callNodeContextMenuIcon {
filter: invert(1);
Expand Down Expand Up @@ -35,10 +37,7 @@
margin-inline: 2px;
}

.react-contextmenu-item--selected > .callNodeContextMenuLabel {
color: #fff;
}

.react-contextmenu-item--selected > .callNodeContextMenuLabel,
.react-contextmenu-item:hover .callNodeContextMenuLabel,
.react-contextmenu-item:active .callNodeContextMenuLabel {
color: #fff;
Expand All @@ -49,7 +48,7 @@
}

.callNodeContextMenuWithKey > div {
/* Verticially align the text with the icon */
/* Vertically align the text with the icon */
display: flex;
flex: 1;
}
Expand Down
16 changes: 8 additions & 8 deletions src/components/shared/CallNodeContextMenu.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */

// @flow
import React, { PureComponent, Fragment } from 'react';
import * as React from 'react';
import { MenuItem } from 'react-contextmenu';
import { ContextMenu } from './ContextMenu';
import explicitConnect from 'firefox-profiler/utils/connect';
Expand Down Expand Up @@ -64,7 +64,7 @@ type Props = ConnectedProps<{||}, StateProps, DispatchProps>;

import './CallNodeContextMenu.css';

class CallNodeContextMenuImpl extends PureComponent<Props> {
class CallNodeContextMenuImpl extends React.PureComponent<Props> {
_hidingTimeout: TimeoutID | null = null;

// Using setTimeout here is a bit complex, but is necessary to make the menu
Expand Down Expand Up @@ -446,7 +446,7 @@ class CallNodeContextMenuImpl extends PureComponent<Props> {
const showExpandAll = selectedTab === 'calltree';

return (
<Fragment>
<>
<TransformMenuItem
shortcut="m"
icon="Merge"
Expand Down Expand Up @@ -569,12 +569,12 @@ class CallNodeContextMenuImpl extends PureComponent<Props> {
<div className="react-contextmenu-separator" />

{showExpandAll ? (
<Fragment>
<>
<MenuItem onClick={this._handleClick} data={{ type: 'expand-all' }}>
Expand all
</MenuItem>
<div className="react-contextmenu-separator" />
</Fragment>
</>
) : null}
<MenuItem onClick={this._handleClick} data={{ type: 'searchfox' }}>
Look up the function name on Searchfox
Expand All @@ -593,7 +593,7 @@ class CallNodeContextMenuImpl extends PureComponent<Props> {
<MenuItem onClick={this._handleClick} data={{ type: 'copy-stack' }}>
Copy stack
</MenuItem>
</Fragment>
</>
);
}

Expand Down Expand Up @@ -662,8 +662,8 @@ export const CallNodeContextMenu = explicitConnect<
});

function TransformMenuItem(props: {|
+children: *,
+onClick: *,
+children: React.Node,
+onClick: (event: SyntheticEvent<>, data: { type: string }) => void,
+transform: string,
+shortcut: string,
+icon: string,
Expand Down
10 changes: 5 additions & 5 deletions src/test/components/CallNodeContextMenu.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -27,13 +27,13 @@ describe('calltree/CallNodeContextMenu', function() {
profile,
funcNamesDictPerThread: [{ A, B }],
} = getProfileFromTextSamples(`
A A A
A A A
B[lib:XUL] B[lib:XUL] B[lib:XUL]
B[lib:XUL] B[lib:XUL] B[lib:XUL]
B[lib:XUL] B[lib:XUL] B[lib:XUL]
C C H
D F I
E E
C C H
D F I
E E
`);
const store = storeWithProfile(profile);

Expand Down Expand Up @@ -113,7 +113,7 @@ describe('calltree/CallNodeContextMenu', function() {
// This test only asserts that a bunch of call nodes were actually expanded.
expect(
selectedThreadSelectors.getExpandedCallNodeIndexes(getState())
).toHaveLength(7);
).toHaveLength(11);
});

it('can look up functions on SearchFox', function() {
Expand Down
6 changes: 3 additions & 3 deletions src/test/components/TransformShortcuts.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,11 @@ import { Provider } from 'react-redux';
import { getProfileFromTextSamples } from '../fixtures/profiles/processed-profile';
import { storeWithProfile } from '../fixtures/stores';
import { changeSelectedCallNode } from '../../actions/profile-view';
import FlameGraph from '../../components/flame-graph';
import { FlameGraph } from '../../components/flame-graph';
import { selectedThreadSelectors } from 'firefox-profiler/selectors';
import { ensureExists } from '../../utils/flow';
import { fireFullKeyPress } from '../fixtures/utils';
import ProfileCallTreeView from '../../components/calltree/ProfileCallTreeView';
import { ProfileCallTreeView } from '../../components/calltree/ProfileCallTreeView';
import type { TransformType } from 'firefox-profiler/types';

type TestSetup = {|
Expand Down Expand Up @@ -70,7 +70,7 @@ function testTransformKeyboardShortcuts(setup: () => TestSetup) {
expect(getTransformType()).toEqual('collapse-function-subtree');
});

it('ignores shorcuts with modifiers', () => {
it('ignores shortcuts with modifiers', () => {
const { pressKey, getTransformType } = setup();
pressKey({ key: 'c', ctrlKey: true });
pressKey({ key: 'c', metaKey: true });
Expand Down
12 changes: 11 additions & 1 deletion src/test/fixtures/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -329,7 +329,17 @@ export function fireFullContextMenu(
/**
* React Testing Library only sends one event at a time, but a lot of component logic
* assumes that events come in a natural cascade. This utility ensures that cascasde
* gets fired correctly.
* gets fired more correctly.
*
* Note, that this utility is not quite complete in it's implementation. There are
* more complex interactions with prevent defaulting, and passing in the correct
* keycode information.
*
* For a more complete implementation see:
* https://github.com/testing-library/user-event/blob/c187639cbc7d2651d3392db6967f614a75a32695/src/type.js#L283
*
* And addition Julien's review comments here:
* https://github.com/firefox-devtools/profiler/pull/2842/commits/6be20eb6eafca56644b1d55010cc1824a1d03695#r501061693
*/
export function fireFullKeyPress(element: HTMLElement, options?: *) {
fireEvent.keyDown(element, options);
Expand Down

0 comments on commit 755b7ad

Please sign in to comment.