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: Create palette from JSON file (resolves #1) #2

Merged
merged 58 commits into from
Dec 20, 2023
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
58 commits
Select commit Hold shift + click to select a range
43ba41e
feat: create palette from JSON file
klown Oct 2, 2023
2485833
fix: remove extra render() call
klown Oct 2, 2023
f1ac9e0
fix: add copyright and licence comment
klown Oct 2, 2023
9542e87
lint: remove errant import of `waitFor`
klown Oct 2, 2023
808b7d8
fix: switch to `.scss` (sass) for styles
klown Oct 4, 2023
47ab98c
refactor: group styles for `paletteCell` class
klown Oct 4, 2023
3906b19
fix: suppress implicit role (graphic) of `svg` elements in a palette …
klown Oct 4, 2023
25ab172
feat: use options block in json definition of palette cells
klown Oct 4, 2023
98aecfb
chore: use string constant for svg markup
klown Oct 11, 2023
a1a318d
chore: move rendering code for palette to `index.html`
klown Oct 12, 2023
7a2c857
chore: pass `options` block from json instead of individual properties
klown Oct 12, 2023
126de89
fix: add latest bmw palette json definition file with proper layout
klown Oct 13, 2023
d3db085
fix: pass `type` of bmw cell to `PaletteCell` component
klown Oct 13, 2023
3592783
fix: spelling error
klown Oct 13, 2023
ea5e80c
fix: lint error
klown Oct 13, 2023
a0080ca
feat: define TypesScript Type for PaletteCell `props` argument
klown Oct 13, 2023
a58d0fb
feat: use `SVGBuilder` to fetch svg to render within the cell
klown Oct 16, 2023
58ed03d
feat: make use of the svg builder codes in the latest blissary-bci-id…
klown Oct 17, 2023
43eafe7
feat: replace Bliss-Blissary-BCI-ID-Map submdodule with fetch()
klown Oct 18, 2023
77c240e
feat: share-able `BlissSymbol` component that combines svg and text l…
klown Oct 19, 2023
6be1737
feat: remove extra style and classes from `PaletteCell`'s `props` arg…
klown Oct 19, 2023
fc594f6
fix: remove debug statement and temporary comment
klown Oct 20, 2023
1088f9e
feat: add blissary id map code that loads the remote map
klown Oct 20, 2023
6a32931
fix: remove tests no longer relevant due to change in `PaletteCellPro…
klown Oct 20, 2023
4b05f39
feat: renamed `PaletteCell` to `ActionBmwCodeCell`
klown Oct 20, 2023
fd3f054
feat: renamed `type` value to `ActionBmwCodeCell` in `bmw_palette.json`
klown Oct 20, 2023
0b54137
feat: add a map for retrieving the component when rendering a cell type
cindyli Oct 20, 2023
fdb3d67
fix: remove a test file
cindyli Oct 20, 2023
8b7f61d
fix: add error handling and improve docs
cindyli Oct 21, 2023
f2b4048
fix: address review comments
cindyli Oct 23, 2023
0639ece
fix: remove a duplicate "of"
cindyli Oct 23, 2023
406d52f
Merge pull request #1 from cindyli/feat/add-cellType-registry
klown Oct 23, 2023
cb4afb8
feat: install BlissSVGBuilder from npm
klown Oct 24, 2023
3cbdbee
feat: remove top-level (aka, module level) await
klown Oct 30, 2023
278a67e
feat: modify jest config to include global fetch
klown Oct 30, 2023
bc98779
feat: add fetch() back for jest by using whatwg-fetch module
klown Oct 30, 2023
106fb01
chore: upgrade to bliss-svg-builder v0.1.0-alpha.4
klown Oct 31, 2023
7900134
fix: move location of `try {` statement to handle more errors
klown Nov 2, 2023
380dd91
chore: use latest `bmw_palette.json` from the baby-bliss-bot project
klown Nov 3, 2023
b398c6b
feat: add tests for BlissSymbol component
klown Nov 17, 2023
0a97b2a
chore: split the two `BlissSymbol` tests into two separate `test(...);`s
klown Nov 24, 2023
f5e7136
chore: rework `ActionBmwCodeCell` tests
klown Nov 24, 2023
da78a8b
feat: add aria markup for the <svg> element
klown Nov 24, 2023
36f197e
fix: remove lint exclusion for Bliss-Blissary-BCI-ID-Map folder
klown Nov 27, 2023
2819b29
chore: add comment regarding rationale for using whatwg-fetch in tes…
klown Nov 27, 2023
8118cd7
fix: use `rem` instead of `em` for styling
klown Nov 28, 2023
0c73c0d
chore: add tests for the `Palette` component
klown Nov 29, 2023
d75020b
feat: move main script to `index.js` and include it within `index.html`
klown Nov 29, 2023
4c20fcd
chore: remove debugging information
klown Nov 29, 2023
eeb4e77
fix: improve documentation for how to create palettes
klown Nov 29, 2023
e252bea
fix: add test for `blissaryIdMap()` when given unknown bci-av-id
klown Nov 29, 2023
7dfdf63
fix: add type declarations for known types
klown Nov 30, 2023
184498a
fix: remove disable styles
klown Nov 30, 2023
8038d14
fix: remove debug info for async loading of the Blissary-BCI-ID-Map
klown Dec 1, 2023
dd57a24
feat: remove question-mark svg for unknown bci-av-id
klown Dec 1, 2023
d0d5237
fix: change destructuring assignment to one statement
klown Dec 1, 2023
cabf34d
feat: consolidate type definitions into one shared file
klown Dec 14, 2023
352ce6c
fix: move property type definitions, as private, to their functional …
klown Dec 20, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
21 changes: 12 additions & 9 deletions index.html
Original file line number Diff line number Diff line change
@@ -1,12 +1,15 @@
<!DOCTYPE html>
<html lang="en">
<head>
<meta charset="UTF-8" />
<meta name="viewport" content="width=device-width, initial-scale=1.0" />
<meta name="color-scheme" content="light dark" />
<title>Adaptive Palette</title>
</head>
<body>
<div id="app"></div>
</body>
<head>
<meta charset="UTF-8" />
<meta name="viewport" content="width=device-width, initial-scale=1.0" />
<meta name="color-scheme" content="light dark" />
<title>Adaptive Palette</title>
</head>
<body>
<h2>Palette Based on JSON</h1>
<div id="paletteCell">
Copy link
Contributor

Choose a reason for hiding this comment

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

This div is used to render the entire palette. It might be better to use an id "palette".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The paletteCell id was left over from when the code only rendered a single PaletteCell. I have changed it to bmwKeyCodes for now, but I would not be surprised if it's changed again.

I also cleaned up Palette.ts by removing the rendering code and moving it to index.html.

<script type="module" src="/src/Palette.ts"></script>
</div>
</body>
</html>
5 changes: 4 additions & 1 deletion jest.config.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -10,5 +10,8 @@ module.exports = {
},
testPathIgnorePatterns: ["/node_modules/", "<rootDir>/dist/"],
transformIgnorePatterns: ["<rootDir>/dist/"],
moduleFileExtensions: ["mjs", "js", "jsx", "ts", "tsx", "json", "node"]
moduleFileExtensions: ["mjs", "js", "jsx", "ts", "tsx", "json", "node"],
moduleNameMapper: {
"^.+\\.(css|less|scss)$": "babel-jest"
}
};
18 changes: 18 additions & 0 deletions src/Palette.scss
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
/*
* Copyright 2023 Inclusive Design Research Centre, OCAD University
* All rights reserved.
*
* Licensed under the New BSD license. You may not use this file except in
* compliance with this License.
*
* You may obtain a copy of the License at
* https://github.com/inclusive-design/adaptive-palette/blob/main/LICENSE
*/

.paletteContainer {
display: grid;
grid-template-columns: auto auto auto;
border: 2px solid #f76707;
border-radius: 5px;
background-color: pink;
}
75 changes: 75 additions & 0 deletions src/Palette.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
/*
* Copyright 2023 Inclusive Design Research Centre, OCAD University
* All rights reserved.
*
* Licensed under the New BSD license. You may not use this file except in
* compliance with this License.
*
* You may obtain a copy of the License at
* https://github.com/inclusive-design/adaptive-palette/blob/main/LICENSE
*/

import { render } from "preact";
import { html } from "htm/preact";
import { PaletteCell } from "./PaletteCell";
import "./Palette.scss";

/**
* Given a palette defined in a json structure, compute the number of rows
* and columns in that palette.
*
* @param {Object} paletteDefinition - An object that lists the positions,
* heights and widths of the cells in the palette.
* @return {Object} - The row and column counts: `{ numRows: ..., numColumns: ...}`.
*/
function countRowsColumns (paletteDefinition) {
let rowCount = 0;
let colCount = 0;
let rightColumn = 0;
let bottomRow = 0;
const cellIds = Object.keys(paletteDefinition.cells);
cellIds.forEach((id) => {
const cellOptions = paletteDefinition.cells[id].options;
rightColumn = cellOptions.columnStart + cellOptions.columnSpan;
if (rightColumn > colCount) {
colCount = rightColumn;
}
bottomRow = cellOptions.rowStart + cellOptions.rowSpan;
if (bottomRow > rowCount) {
rowCount = bottomRow;
}
});
return { numRows: rowCount, numColumns: colCount };
}

export function Palette (props) {
Copy link
Contributor

Choose a reason for hiding this comment

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

A typescript type can be defined to verify the data structure of props.

The same comment applies to other component props.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was going to write an interface for the props arguments, and give them a better name, but I was putting that off until the structure was better defined. So, I will define that interface at some point.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a test for the Palette 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.

Done, but only for the Palette component itself. It would be good to test the countRowsColumns(). However it's not exported. It's a "private" function. I've run into that issue before, and have solved it in the past by exporting the function. That feels like a hack, however, and I looked for another way.

There is a simple solution given in a stackoverflow article. The solution is to create and export a structure named exportForTesting that contains the private functions. This structure is imported only in the test code and ignored by production code. For example:

// In the implementation file: 
function countRowsColumns (...) { ... };
...
export exportForTesting = { countRowsColumns };

// In the test code:
import { exportForTesting, Palette } from "./Palette.ts";
const { countRowsColumns } = exportForTesting;
...

It's less of a hack. Additional searching led to an article about the subject and from there to a plugin called babel-plugin-rewire. Rewire allows test code, and only test code, to have access to private variables. But, I have only started to look into it. Rewire is more general and covers more that just access to private variables in tests. It might be overkill.


const paletteDefinition = props.json;
const rowsCols = countRowsColumns(paletteDefinition);
const cellIds = Object.keys(paletteDefinition.cells);

// Loop to create an array of renderings for each cell
const theCells = [];
cellIds.forEach((id) => {
const aCell = paletteDefinition.cells[id];
const cellOptions = aCell.options;
const paletteCell = html`
<${PaletteCell} id="${id}" labelText="${cellOptions.label}"
Copy link
Contributor

Choose a reason for hiding this comment

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

The cellOptions can be passed as one single prop so that when this line can be shared for instantiating various components of various cell types:

<${PaletteCell} id="${id}" options=${cellOptions} />

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

I've also incorporated your latest bmw_palette.json with the correct layout of the BMW code keys. I noticed that the type of key is not within the options block. I have no opinion if it should be or not, but I'm passing it along like so:

<${PaletteCell} id="${id}" options=${cellOptions} type=${aCell.type} />

At the moment, PaletteCell does nothing with the type, but I presume it will be used when there is a component registry that maps type strings to components.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Replying to myself: I'm at the wrong level. If there is a registry, the type will be used by the Palette component, something like:

<${aCell.type} id="${id}" options=${cellOptions} />

There is probably no reason to pass the type to the PaletteCell component. I'll take it out at some point.

columnStart="${cellOptions.columnStart}" columnSpan="${cellOptions.columnSpan}"
rowStart="${cellOptions.rowStart}" rowSpan="${cellOptions.rowSpan}"
/>`;
theCells.push(paletteCell);
});

return html`
<div
class="paletteContainer"
style="grid-template-columns: repeat(${rowsCols.numColumns}, auto);">
${theCells}
</div>
`;
}

import bmwJson from "./keyboards/bmw_palette.json";
render (html`<${Palette} json=${bmwJson}/>`, document.getElementById("paletteCell"));

35 changes: 35 additions & 0 deletions src/PaletteCell.scss
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
/*
* Copyright 2023 Inclusive Design Research Centre, OCAD University
* All rights reserved.
*
* Licensed under the New BSD license. You may not use this file except in
* compliance with this License.
*
* You may obtain a copy of the License at
* https://github.com/inclusive-design/adaptive-palette/blob/main/LICENSE
*/

.paletteCell {
border: 2px solid blue;
background-color: gray;
border-radius: 5px;
padding: 1em;
font-size: 1em;
text-align: center;

&:hover, &:focus {
background-color: lightblue;
color: #0000aa;
}

& > svg {
display: block;
}
}

.disabled {
opacity: 0.6;
color: #090909;
cursor: not-allowed;
}

84 changes: 84 additions & 0 deletions src/PaletteCell.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
/*
* Copyright 2023 Inclusive Design Research Centre, OCAD University
* All rights reserved.
*
* Licensed under the New BSD license. You may not use this file except in
* compliance with this License.
*
* You may obtain a copy of the License at
* https://github.com/inclusive-design/adaptive-palette/blob/main/LICENSE
*/

import { render, screen, fireEvent } from "@testing-library/preact";
import "@testing-library/jest-dom";
import { html } from "htm/preact";
import { PaletteCell } from "./PaletteCell";

test("The PaletteCell is rendered correctly", async () => {
render(html`
<${PaletteCell}
id="uuid-of-some-kind"
labelText="Bliss Language"
rowStart="3"
rowSpan="2"
columnStart="2"
columnSpan="1"
style="background-color: green;"
/>`);

let button = await screen.findByRole("button", {name: "Bliss Language"});
let buttonStyles = window.getComputedStyle(button);
console.log(`Button's background colour: ${buttonStyles.backgroundColor}`);

// Check that the PaletteCell/button is rendered and has the correct
// attributes and text
expect(button).toBeVisible();
expect(button).toBeValid();
expect(button.id).toBe("uuid-of-some-kind");

// PaletteCell.css does not specify a bacground colour. The background will
// be whatever the browser default is. For now, check that the specified
// colour in the test render above is correct
expect(button.style["background-color"]).toBe("green");
expect(button.style["grid-column"]).toBe("2 / span 1");
expect(button.style["grid-row"]).toBe("3 / span 2");
expect(button.textContent).toBe("Bliss Language");

// Check disabled state (should be enabled)
expect(button.getAttribute("disabled")).toBe(null);
expect(button.getAttribute("class")).not.toContain("disabled");

// Check styling due to mouse hover
// TODO: get `:hover` style from PaletteCell.css?
fireEvent.mouseOver(button);

buttonStyles = window.getComputedStyle(button);
console.log(`Button's background colour: ${buttonStyles.backgroundColor}`);
//expect(buttonStyles.backgroundColor).toBe("#cccccc");
//expect(buttonStyles.color).toBe("#0000aa");

fireEvent.mouseLeave(button);
buttonStyles = window.getComputedStyle(button);
console.log(`Button's background colour: ${buttonStyles.backgroundColor}`);
//expect(buttonStyles.backgroundColor).not.toBe("#cccccc");
//expect(buttonStyles.color).not.toBe("#0000aa");

// Create a diabled button and check its disabled state.
render(html`
<${PaletteCell}
id="uuid-of-another-kind"
labelText="Disabled Cell"
rowStart="3"
rowSpan="2"
columnStart="2"
columnSpan="1"
class="disabled"
/>`);

button = await screen.findByRole("button", {name: "Disabled Cell"});
expect(button).toBeVisible();
expect(button).toBeValid();
expect(button.id).toBe("uuid-of-another-kind");
expect(button.getAttribute("disabled")).toBe("");
expect(button.getAttribute("class")).toContain("disabled");
});
45 changes: 45 additions & 0 deletions src/PaletteCell.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
/*
* Copyright 2023 Inclusive Design Research Centre, OCAD University
* All rights reserved.
*
* Licensed under the New BSD license. You may not use this file except in
* compliance with this License.
*
* You may obtain a copy of the License at
* https://github.com/inclusive-design/adaptive-palette/blob/main/LICENSE
*/

import { html } from "htm/preact";
import "./PaletteCell.scss";

export function PaletteCell (props) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Regarding the component name, I was thinking it uses the same name as the cell type it supports. The name starts with a more general category performed by the cell. For example, keys that perform actions when pressed are in "action" category, so this cell type and component for BMW code keys are named "ActionBmwCodeKeys"; names for command buttons start with "Command..."; names for display areas start with "Content...". There might be better names than "action", "command" and "content". This naming convention will make files in the /src folder easy to navigate since all components in the same category will stay together. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like the idea, especially the way that files are grouped within the /src folder. "Command" and "Content" are fine, but "Action" is too generic, I think. I'm leaning towards "Insert" since the action is to insert content into encoding area and, ultimately, into the message area.

Still, there is nothing within "Insert" to communicate the idea of entering a sequence of codes. Then again, I'm not sure there needs to be.


// Basic styles are the `paletteCell` class defined in PaletteCell.css.
// Concatenate any additional classes provided by `props`.
let classes = "paletteCell";
if (props.class) {
Copy link
Contributor

Choose a reason for hiding this comment

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

There isn't yet class and styles property. Would it be too early to write code to handle them?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd say it was too early to remove it :-)

I am using an extra class for when the cell is disabled, and this use is shown in the tests, PaletteCell.test.ts, like so:

<${PaletteCell} id="${id}" options=${cellOptions} class="disabled" />

That is handled within PaletteCell by:

  • setting the button's disabled attribute (<button disabled ...>)
  • adding the disabled class to the button so the disabled styles are used ( (<button disabled class="paletteCell disabled" ...>

However, having said that, I don't know how disabled is actually handled on the palette. We know that as bmw codes are entered, certain keys are disabled since they are irrelevant for the current code sequence. How is that handled by Preact? This is a case of tracking state and probably involves hooks but I do not know the details. I will have a look at the documentation.

classes = `${classes} ${props.class}`;
}
let disabled = false;
if (classes.indexOf("disabled") >= 0) {
disabled = true;
}

// Also concatenate local styles with given grid cell styles
let styles = `
grid-column: ${props.columnStart} / span ${props.columnSpan};
grid-row: ${props.rowStart} / span ${props.rowSpan};
`;
if (props.style) {
styles = `${styles} ${props.style}`;
}

return html`
<button id="${props.id}" class="${classes}" style="${styles}" disabled=${disabled}>
<svg version="1.1" xmlns="http://www.w3.org/2000/svg" viewBox="0 0 10 10" role="presentation">
<circle cx="5" cy="5" r="4" fill="transparent" stroke="black" stroke-width="1"/>
</svg>
${props.labelText}
</button>
`;
}
53 changes: 53 additions & 0 deletions src/PaletteStore.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
/*
* Copyright 2023 Inclusive Design Research Centre, OCAD University
* All rights reserved.
*
* Licensed under the New BSD license. You may not use this file except in
* compliance with this License.
*
* You may obtain a copy of the License at
* https://github.com/inclusive-design/adaptive-palette/blob/main/LICENSE
*/

"use strict";

import { PaletteStore } from "./PaletteStore";

describe("PaletteStore module", () => {

const dummyPalette1 = { name: "dummyPalette1", palette: null };
const dummyPalette2 = { name: "dummyPalette2", palette: null };
const dummyPalette2Name = "DummyPalette2";

const paletteStore = new PaletteStore();

test("Empty PaletteStore", () => {
expect(paletteStore.isEmpty()).toBe(true);
});

test("Non-empty PaletteStore", () => {
paletteStore.addPalette(dummyPalette1);
expect(paletteStore.isEmpty()).toBe(false);
expect(paletteStore.numPalettes).toBe(1);
expect(paletteStore.paletteList).toEqual(["dummyPalette1"]);
});

test("Add another palette", () => {
paletteStore.addPalette(dummyPalette2, dummyPalette2Name);
expect(paletteStore.numPalettes).toBe(2);
expect(paletteStore.paletteList).toEqual(["dummyPalette1", dummyPalette2Name]);
});

test("Retrieve a palette", () => {
const retrievedPalette = paletteStore.getNamedPalette(dummyPalette2Name);
expect(retrievedPalette).toBe(dummyPalette2);
});

test("Delete a palette", () => {
const numPalettes = paletteStore.numPalettes;
const removedPalette = paletteStore.removePalette(dummyPalette1.name);
expect(removedPalette).toBe(dummyPalette1);
expect(paletteStore.numPalettes).toBe(numPalettes - 1);
expect(paletteStore.getNamedPalette(dummyPalette1.name)).toBeUndefined();
});
});