Commit 9c7902f6 authored by Jacques's avatar Jacques

Address code review comments

Addressed general code review comments
parent c191dfbb
...@@ -22,7 +22,7 @@ export default { ...@@ -22,7 +22,7 @@ export default {
...d, ...d,
definitionLineNumber: parseInt(d.definition_path?.split('#L').pop() || 0, 10), definitionLineNumber: parseInt(d.definition_path?.split('#L').pop() || 0, 10),
}; };
addInteractionClass(path, d, state.wrapTextNodes); addInteractionClass({ path, d, wrapTextNodes: state.wrapTextNodes });
} }
return acc; return acc;
}, {}); }, {});
...@@ -35,7 +35,7 @@ export default { ...@@ -35,7 +35,7 @@ export default {
showBlobInteractionZones({ state }, path) { showBlobInteractionZones({ state }, path) {
if (state.data && state.data[path]) { if (state.data && state.data[path]) {
Object.values(state.data[path]).forEach((d) => Object.values(state.data[path]).forEach((d) =>
addInteractionClass(path, d, state.wrapTextNodes), addInteractionClass({ path, d, wrapTextNodes: state.wrapTextNodes }),
); );
} }
}, },
......
...@@ -5,7 +5,7 @@ export const cachedData = new Map(); ...@@ -5,7 +5,7 @@ export const cachedData = new Map();
export const getCurrentHoverElement = () => cachedData.get('current'); export const getCurrentHoverElement = () => cachedData.get('current');
export const setCurrentHoverElement = (el) => cachedData.set('current', el); export const setCurrentHoverElement = (el) => cachedData.set('current', el);
export const addInteractionClass = (path, d, wrapTextNodes) => { export const addInteractionClass = ({ path, d, wrapTextNodes }) => {
const lineNumber = d.start_line + 1; const lineNumber = d.start_line + 1;
const lines = document const lines = document
.querySelector(`[data-path="${path}"]`) .querySelector(`[data-path="${path}"]`)
......
...@@ -506,10 +506,6 @@ span.idiff { ...@@ -506,10 +506,6 @@ span.idiff {
text-indent: -$grid-size * 2; text-indent: -$grid-size * 2;
} }
.blob-viewer .code-navigation-popover .code {
text-indent: $grid-size * 2;
}
.tree-item-link { .tree-item-link {
&:not(.is-submodule) { &:not(.is-submodule) {
span { span {
......
...@@ -111,15 +111,15 @@ describe('Code navigation actions', () => { ...@@ -111,15 +111,15 @@ describe('Code navigation actions', () => {
[], [],
) )
.then(() => { .then(() => {
expect(addInteractionClass).toHaveBeenCalledWith( expect(addInteractionClass).toHaveBeenCalledWith({
'index.js', path: 'index.js',
{ d: {
start_line: 0, start_line: 0,
start_char: 0, start_char: 0,
hover: { value: '123' }, hover: { value: '123' },
}, },
wrapTextNodes, wrapTextNodes,
); });
}) })
.then(done) .then(done)
.catch(done.fail); .catch(done.fail);
...@@ -157,8 +157,12 @@ describe('Code navigation actions', () => { ...@@ -157,8 +157,12 @@ describe('Code navigation actions', () => {
expect(addInteractionClass).toHaveBeenCalled(); expect(addInteractionClass).toHaveBeenCalled();
expect(addInteractionClass.mock.calls.length).toBe(2); expect(addInteractionClass.mock.calls.length).toBe(2);
expect(addInteractionClass.mock.calls[0]).toEqual(['index.js', 'test', wrapTextNodes]); expect(addInteractionClass.mock.calls[0]).toEqual([
expect(addInteractionClass.mock.calls[1]).toEqual(['index.js', 'console.log', wrapTextNodes]); { path: 'index.js', d: 'test', wrapTextNodes },
]);
expect(addInteractionClass.mock.calls[1]).toEqual([
{ path: 'index.js', d: 'console.log', wrapTextNodes },
]);
}); });
it('does not call addInteractionClass when no data exists', () => { it('does not call addInteractionClass when no data exists', () => {
......
...@@ -49,7 +49,7 @@ describe('addInteractionClass', () => { ...@@ -49,7 +49,7 @@ describe('addInteractionClass', () => {
`( `(
'it sets code navigation attributes for line $line and character $char', 'it sets code navigation attributes for line $line and character $char',
({ line, char, index }) => { ({ line, char, index }) => {
addInteractionClass('index.js', { start_line: line, start_char: char }); addInteractionClass({ path: 'index.js', d: { start_line: line, start_char: char } });
expect(document.querySelectorAll(`#LC${line + 1} span`)[index].classList).toContain( expect(document.querySelectorAll(`#LC${line + 1} span`)[index].classList).toContain(
'js-code-navigation', 'js-code-navigation',
...@@ -57,18 +57,30 @@ describe('addInteractionClass', () => { ...@@ -57,18 +57,30 @@ describe('addInteractionClass', () => {
}, },
); );
it('wraps text nodes and spaces', () => { describe('wrapTextNodes', () => {
setFixtures( beforeEach(() => {
'<div data-path="index.js"><div class="blob-content"><div id="LC1" class="line"> Text </div></div></div>', setFixtures(
); '<div data-path="index.js"><div class="blob-content"><div id="LC1" class="line"> Text </div></div></div>',
);
});
const params = { path: 'index.js', d: { start_line: 0, start_char: 0 } };
const findAllSpans = () => document.querySelectorAll('#LC1 span');
addInteractionClass('index.js', { start_line: 0, start_char: 0 }, true); it('does not wrap text nodes by default', () => {
addInteractionClass(params);
const spans = findAllSpans();
expect(spans.length).toBe(0);
});
const spans = document.querySelectorAll(`#LC1 span`); it('wraps text nodes if wrapTextNodes is true', () => {
addInteractionClass({ ...params, wrapTextNodes: true });
const spans = findAllSpans();
expect(spans.length).toBe(3); expect(spans.length).toBe(3);
expect(spans[0].textContent).toBe(' '); expect(spans[0].textContent).toBe(' ');
expect(spans[1].textContent).toBe('Text'); expect(spans[1].textContent).toBe('Text');
expect(spans[2].textContent).toBe(' '); expect(spans[2].textContent).toBe(' ');
});
}); });
}); });
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment