Skip to content
Open
Show file tree
Hide file tree
Changes from all 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
Original file line number Diff line number Diff line change
Expand Up @@ -92,34 +92,6 @@ describe('SafeHtml5RendererIndex', () => {
});
});

describe('windowSizeClass pass-through', () => {
test("the 'small-window' class is applied to table caption when innerWidth <= 600", async () => {
Object.defineProperty(window, 'innerWidth', {
writable: true,
configurable: true,
value: 500,
});
renderComponent();
await waitFor(() => {
const caption = screen.getByText('Mocked 3-column Table');
expect(caption).toHaveClass('small-window');
});
});

test("the 'small-window' class is not applied to table caption when innerWidth > 600", async () => {
Object.defineProperty(window, 'innerWidth', {
writable: true,
configurable: true,
value: 800,
});
renderComponent();
await waitFor(() => {
const caption = screen.getByText('Mocked 3-column Table');
expect(caption).not.toHaveClass('small-window');
});
});
});

describe('progress tracking', () => {
test('emits startTracking on created', async () => {
const { emitted } = renderComponent();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,12 +15,7 @@
role="region"
:aria-label="$tr('articleContent')"
>
<SafeHTML
:html="html"
:styleOverrides="{
windowSizeClass: windowSizeClass,
}"
/>
<SafeHTML :html="html" />
</div>
</div>

Expand All @@ -32,26 +27,19 @@
import ZipFile from 'kolibri-zip';
import SafeHTML from 'kolibri-common/components/SafeHTML';
import useContentViewer, { contentViewerProps } from 'kolibri/composables/useContentViewer';
import useKResponsiveWindow from 'kolibri-design-system/lib/composables/useKResponsiveWindow';
import { computed } from 'vue';

export default {
name: 'SafeHtml5RendererIndex',
components: {
SafeHTML,
},
setup(props, context) {
const { windowIsSmall } = useKResponsiveWindow();
const windowSizeClass = computed(() => {
return windowIsSmall.value ? 'small-window' : '';
});
const { defaultFile, forceDurationBasedProgress, durationBasedProgress } = useContentViewer(
props,
context,
{ defaultDuration: 300 },
);
return {
windowSizeClass,
defaultFile,
forceDurationBasedProgress,
durationBasedProgress,
Expand Down
54 changes: 0 additions & 54 deletions packages/kolibri-common/components/SafeHTML/SafeHtmlTable.js

This file was deleted.

135 changes: 135 additions & 0 deletions packages/kolibri-common/components/SafeHTML/SafeHtmlTable.vue
Original file line number Diff line number Diff line change
@@ -0,0 +1,135 @@
<template>

<div
class="table-container"
data-testid="table-container"
>
<table
:style="tableStyle"
v-bind="attributes"
>
<component :is="renderTableContent" />
</table>
</div>

</template>


<script>

const { ELEMENT_NODE } =
typeof window !== 'undefined' && window.Node ? window.Node : { ELEMENT_NODE: 1 };

export default {
name: 'SafeHtmlTable',
props: {
node: {
required: true,
validator: prop => typeof prop === 'object' && prop !== null,
},
attributes: {
type: Object,
required: true,
},
mapNode: {
type: Function,
required: true,
},
mapChildren: {
type: Function,
required: true,
},
},

computed: {
tableStyle() {
const firstRow = this.node.querySelector && this.node.querySelector('tr');
const colCount = firstRow ? firstRow.children.length : 0;
let tableWidth = '640px';
if (colCount > 3) {
tableWidth = `${colCount * 200}px`;
}

return { width: tableWidth };
},

renderTableContent() {
const vm = this;
return {
functional: true,
render(h) {
Copy link
Member

Choose a reason for hiding this comment

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

This implementation looks correct, but architecturally it introduces a potential source of confusion. We’re creating a functional component inside a computed property of a regular component—essentially building a mini component-factory within the component. This adds layers of complexity we’re trying to move away from.

A cleaner alternative would be to generate the table elements dynamically (not programmatically) and then assigning relevant styling defined within the component itself, thus the redundancy of the style override props.

Our goal is to migrate the current functional component (.js) into an Options API or, ideally, a composable-based component structure. This will help keep the codebase more consistent and maintainable going forward.

const tableChildren = [];
Copy link
Member

Choose a reason for hiding this comment

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

Now that we're just adding safe-html class to the caption - couldn't we just use the passed in mapChildren function here to map the children and then return them - because it already does that!


for (let i = 0; i < vm.node.childNodes.length; i++) {
const childNode = vm.node.childNodes[i];

if (vm.isCaption(childNode)) {
const captionAttrs = vm.getCaptionAttrs(childNode);
const captionChildren = vm.mapChildren(childNode.childNodes);

tableChildren.push(
h(
'caption',
{
attrs: captionAttrs,
class: ['safe-html', captionAttrs.class].filter(Boolean),
},
captionChildren,
),
);
} else if (vm.isElementNode(childNode)) {
tableChildren.push(vm.mapNode(childNode));
}
}

return tableChildren;
},
};
},
},

methods: {
isCaption(node) {
return (
node &&
node.nodeType === ELEMENT_NODE &&
typeof node.tagName === 'string' &&
node.tagName.toLowerCase() === 'caption'
);
},

isElementNode(node) {
return node && node.nodeType === ELEMENT_NODE;
},

getCaptionAttrs(node) {
const captionAttrs = {};

for (const attr of node.attributes) {
const name = attr.name.toLowerCase();
if (name === 'id') continue;
captionAttrs[attr.name] = attr.value;
}

return captionAttrs;
},
},
};

</script>


<style scoped>

.table-container {
margin: 1em 0;
overflow-x: auto;
}

caption.safe-html {
padding: 2px 0 4px;
font-weight: bold;
text-align: center;
}

</style>
Original file line number Diff line number Diff line change
@@ -1,12 +1,10 @@
import { render, screen } from '@testing-library/vue';
import { h } from 'vue';
import SafeHtmlTable from '../SafeHtmlTable.js';
import SafeHtmlTable from '../SafeHtmlTable.vue';

// Create a table element with m rows and n columns
const createSampleNode = (m, n) => {
const table = document.createElement('table');

// Caption
const caption = document.createElement('caption');
caption.textContent = 'Sample Caption';
table.appendChild(caption);
Expand All @@ -15,7 +13,6 @@ const createSampleNode = (m, n) => {
return table;
}

// Thead (row 1)
const thead = document.createElement('thead');
const theadRow = document.createElement('tr');
for (let col = 1; col <= n; col++) {
Expand Down Expand Up @@ -57,27 +54,14 @@ const createSampleNode = (m, n) => {
};

const sampleAttributes = { class: 'safe-html' };
const sampleTableCounter = 6;
const sampleWindowSizeClass = 'small-window';

const mapNode = jest.fn(node => {
if (node.nodeType === Node.ELEMENT_NODE) {
return h(node.tagName.toLowerCase(), { attrs: sampleAttributes }, mapChildren(node.childNodes));
} else if (node.nodeType === Node.TEXT_NODE && node.textContent.trim() !== '') {
return node.textContent;
}
return null;
});

const mapChildren = jest.fn(childNodes => Array.from(childNodes).map(mapNode).filter(Boolean));
const mapNode = jest.fn(() => null);
const mapChildren = jest.fn(() => []);

const renderComponent = (m, n) => {
return render(SafeHtmlTable, {
props: {
node: createSampleNode(m, n),
attributes: sampleAttributes,
tableCounter: sampleTableCounter,
windowSizeClass: sampleWindowSizeClass,
mapNode,
mapChildren,
},
Expand All @@ -98,46 +82,10 @@ describe('SafeHtmlTable', () => {
expect(screen.getByRole('table')).toBeInTheDocument();
});

test('renders the caption', () => {
expect(screen.getByText('Sample Caption')).toBeInTheDocument();
});
});

describe('class and a11y attributes', () => {
beforeEach(() => {
renderComponent(3, 3);
});

test('table has safe-html class', () => {
expect(screen.getByRole('table')).toHaveClass('safe-html');
});

test('caption has safe-html and small-window classes', () => {
expect(screen.getByText('Sample Caption')).toHaveClass('safe-html', 'small-window');
});

test('non-caption child nodes have safe-html class', () => {
const rowGroups = screen.getAllByRole('rowgroup'); // thead, tbody, tfoot
rowGroups.forEach(rowGroup => expect(rowGroup).toHaveClass('safe-html'));

const trs = screen.getAllByRole('row');
trs.forEach(tr => expect(tr).toHaveClass('safe-html'));

const ths = screen.getAllByRole('columnheader');
ths.forEach(th => expect(th).toHaveClass('safe-html'));
const tds = screen.getAllByRole('cell');
tds.forEach(td => expect(td).toHaveClass('safe-html'));
});

test('caption has correct id', () => {
expect(screen.getByText('Sample Caption')).toHaveAttribute('id', 'table-caption-6');
});

test('container div has correct aria-labelledby', () => {
expect(screen.getByTestId('table-container')).toHaveAttribute(
'aria-labelledby',
'table-caption-6',
);
test('renders a caption element', () => {
const table = screen.getByRole('table');
const caption = table.querySelector('caption');
expect(caption).not.toBeNull();
});
});

Expand Down
Loading