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

Feat:-Added open in editor to appear by default #26949

Merged
merged 10 commits into from
Jul 27, 2023
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
116 changes: 116 additions & 0 deletions packages/react-devtools-shared/src/__tests__/ComponentSettings-test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,116 @@
import React from 'react';
import {render, fireEvent} from 'react-dom';
import ComponentsSettings from '../devtools/views/Settings/ComponentsSettings';

describe('ComponentsSettings', () => {
hoxyq marked this conversation as resolved.
Show resolved Hide resolved
let container;

beforeEach(() => {
container = document.createElement('div');
document.body.appendChild(container);
});

afterEach(() => {
document.body.removeChild(container);
container = null;
});

it('should add a filter when "Add filter" button is clicked', () => {
render(<ComponentsSettings />, container);
const addButton = container.querySelector('button');
fireEvent.click(addButton);

// Assert that a new filter is added
expect(container.textContent).toContain('Filter matches');
});

it('should remove a filter when "Delete filter" button is clicked', () => {
render(<ComponentsSettings />, container);
const addButton = container.querySelector('button');
const deleteButton = container.querySelector('button');

fireEvent.click(addButton);
fireEvent.click(deleteButton);

// Assert that the filter is removed
expect(container.textContent).not.toContain('Filter matches');
});

it('should update the collapseNodesByDefault setting when expand/collapse checkbox is clicked', () => {
render(<ComponentsSettings />, container);
const expandCollapseCheckbox = container.querySelector(
'input[type="checkbox"]',
);

fireEvent.click(expandCollapseCheckbox);

// Assert that the setting is updated
expect(expandCollapseCheckbox.checked).toBe(false);
});

it('should update the parseHookNames setting when parse hook names checkbox is clicked', () => {
render(<ComponentsSettings />, container);
const parseHookNamesCheckbox = container.querySelector(
'input[type="checkbox"]',
);

fireEvent.click(parseHookNamesCheckbox);

// Assert that the setting is updated
expect(parseHookNamesCheckbox.checked).toBe(true);
});

it('should update the openInEditorURLPreset and openInEditorURL settings when selecting a different preset', () => {
render(<ComponentsSettings />, container);
const openInEditorURLPresetSelect = container.querySelector('select');
const openInEditorURLInput = container.querySelector('input[type="text"]');

// Select the "VS Code" preset
fireEvent.change(openInEditorURLPresetSelect, {target: {value: 'vscode'}});

// Assert that the preset and URL input are updated
expect(openInEditorURLPresetSelect.value).toBe('vscode');
expect(openInEditorURLInput.value).toBe('vscode://file/{path}:{line}');

// Select the "Custom" preset
fireEvent.change(openInEditorURLPresetSelect, {target: {value: 'custom'}});

// Assert that the preset and URL input are updated
expect(openInEditorURLPresetSelect.value).toBe('custom');
expect(openInEditorURLInput.value).toBe('');

// Update the custom URL input
fireEvent.change(openInEditorURLInput, {
target: {value: 'https://example.com'},
});

// Assert that the custom URL input is updated
expect(openInEditorURLInput.value).toBe('https://example.com');
});

it('should update the filter type when selecting a different type in the filter dropdown', () => {
render(<ComponentsSettings />, container);
const addButton = container.querySelector('button');
fireEvent.click(addButton);

const filterTypeSelect = container.querySelector('select');
fireEvent.change(filterTypeSelect, {target: {value: 'location'}});

// Assert that the filter type is updated
expect(filterTypeSelect.value).toBe('location');
});

it('should update the filter value when entering a value in the filter input field', () => {
render(<ComponentsSettings />, container);
const addButton = container.querySelector('button');
fireEvent.click(addButton);

const filterInput = container.querySelector('input[type="text"]');
fireEvent.change(filterInput, {target: {value: 'example'}});

// Assert that the filter value is updated
expect(filterInput.value).toBe('example');
});

// Add more test cases for other functionality and interactions
});
3 changes: 3 additions & 0 deletions packages/react-devtools-shared/src/constants.js
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,9 @@ export const SESSION_STORAGE_LAST_SELECTION_KEY =
export const LOCAL_STORAGE_OPEN_IN_EDITOR_URL =
'React::DevTools::openInEditorUrl';

export const LOCAL_STORAGE_OPEN_IN_EDITOR_URL_PRESET =
'React::DevTools::openInEditorUrlPreset';

Comment on lines +42 to +44
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest removing this new config, then if the React::DevTools::openInEditorUrl equals to 'vscode://file/{path}:{line}', you select the VSCode option. That will be simpler.

Copy link
Contributor

@Jack-Works Jack-Works Jul 26, 2023

Choose a reason for hiding this comment

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

hi @Biki-das will you consider this? also cc @hoxyq

Copy link
Contributor

Choose a reason for hiding this comment

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

hi @Biki-das will you consider this? also cc @hoxyq

I think this adds kinda wrong dependency, you should select the preset first and then based on its value, you either do nothing or update the link template.

For example, if I select "custom" and then put "vscode://file/{path}:{line}", should we automatically switch to VSCode option and hide input?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hi @Biki-das will you consider this? also cc @hoxyq

I think this adds kinda wrong dependency, you should select the preset first and then based on its value, you either do nothing or update the link template.

For example, if I select "custom" and then put "vscode://file/{path}:{line}", should we automatically switch to VSCode option and hide input?

yup current implementation looks right i think @hoxyq

Copy link
Contributor

Choose a reason for hiding this comment

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

For example, if I select "custom" and then put "vscode://file/{path}:{line}", should we automatically switch to VSCode option and hide input?

Yes. I think this is good, but plz continue this PR if you don't agree with me!

export const LOCAL_STORAGE_PARSE_HOOK_NAMES_KEY =
'React::DevTools::parseHookNames';

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -219,8 +219,11 @@ export default function InspectedElementWrapper(_: Props): React.Node {
}

const url = new URL(editorURL);
url.href = url.href.replace('{path}', source.fileName);
url.href = url.href.replace('{line}', String(source.lineNumber));
url.href = url.href
.replace('{path}', source.fileName)
.replace('{line}', String(source.lineNumber))
.replace('%7Bpath%7D', source.fileName)
.replace('%7Bline%7D', String(source.lineNumber));
window.open(url);
}, [inspectedElement, editorURL]);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,10 @@ import {
useRef,
useState,
} from 'react';
import {LOCAL_STORAGE_OPEN_IN_EDITOR_URL} from '../../../constants';
import {
LOCAL_STORAGE_OPEN_IN_EDITOR_URL,
LOCAL_STORAGE_OPEN_IN_EDITOR_URL_PRESET,
} from '../../../constants';
import {useLocalStorage, useSubscription} from '../hooks';
import {StoreContext} from '../context';
import Button from '../Button';
Expand Down Expand Up @@ -83,11 +86,17 @@ export default function ComponentsSettings(_: {}): React.Node {
[setParseHookNames],
);

const [openInEditorURLPreset, setOpenInEditorURLPreset] = useLocalStorage<
'vscode' | 'custom',
>(LOCAL_STORAGE_OPEN_IN_EDITOR_URL_PRESET, 'custom');

const [openInEditorURL, setOpenInEditorURL] = useLocalStorage<string>(
LOCAL_STORAGE_OPEN_IN_EDITOR_URL,
getDefaultOpenInEditorURL(),
);

const vscodeFilepath = 'vscode://file/{path}:{line}';
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you move it outside of function? Same file, but outside of the component.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks for the review! let me do the same :-)


const [componentFilters, setComponentFilters] = useState<
Array<ComponentFilter>,
>(() => [...store.componentFilters]);
Expand Down Expand Up @@ -280,15 +289,34 @@ export default function ComponentsSettings(_: {}): React.Node {

<label className={styles.OpenInURLSetting}>
Open in Editor URL:{' '}
<input
className={styles.Input}
type="text"
placeholder={process.env.EDITOR_URL ?? 'vscode://file/{path}:{line}'}
value={openInEditorURL}
onChange={event => {
setOpenInEditorURL(event.target.value);
}}
/>
<select
className={styles.Select}
value={openInEditorURLPreset}
onChange={({currentTarget}) => {
const selectedValue = currentTarget.value;
setOpenInEditorURLPreset(selectedValue);
if (selectedValue === 'vscode') {
setOpenInEditorURL(vscodeFilepath);
} else if (selectedValue === 'custom') {
setOpenInEditorURL('');
}
}}>
<option value="vscode">VS Code</option>
<option value="custom">Custom</option>
</select>
{openInEditorURLPreset === 'custom' && (
<input
className={styles.Input}
type="text"
placeholder={
process.env.EDITOR_URL ? process.env.EDITOR_URL : ''
}
value={openInEditorURL}
onChange={event => {
setOpenInEditorURL(event.target.value);
}}
/>
)}
</label>

<div className={styles.Header}>Hide components where...</div>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,7 @@
border: 1px solid var(--color-border);
border-radius: 0.125rem;
padding: 0.125rem;
margin-left: .5rem;
}

.InvalidRegExp,
Expand Down