feat: implement a content display area and associated buttons (resolves #2) #4

cindyli · 2024-01-05T17:11:35Z

No description provided.

…nto feat/create-bmw-encoding

…ents

…nto feat/create-bmw-encoding

…of BMW codes

netlify · 2024-01-05T17:11:40Z

✅ Deploy Preview for adaptive-palette ready!

Name	Link
🔨 Latest commit	`a1badc7`
🔍 Latest deploy log	https://app.netlify.com/sites/adaptive-palette/deploys/65bc027fd1c6d00008269a45
😎 Deploy Preview	https://deploy-preview-4--adaptive-palette.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

klown

This looks great @cindyli! I have a few questions and suggestions.

klown · 2024-01-19T21:36:32Z

.eslintrc.json

@@ -20,6 +20,7 @@
    "node_modules/"
  ],
  "rules": {
+    "@typescript-eslint/no-explicit-any": ["off"],


Is the rationale that if an explicit any is required, then that's allowed? That the developer would need to consciously and judiciously add the any declaration, knowing there was no way around it? I had to do it once in the case of BlissSymbol.ts. In that case I disabled the check for explicit any but just for that one line. Are there are a lot of places where explicit any is used?

Also, why is it not just "off"? Why the array?

Good catch, @klown. Allowing the use of "any" type across the project is dangerous. It's better to do it case by case as what you did with BlissSymbol.ts. I will remove it. I used "any" when working on this PR but removed them at the end. This line is no longer needed.

klown · 2024-01-19T21:37:19Z

src/CommandClearEncoding.ts

+
+  const cellClicked = () => {
+    setFullEncoding([]);
+  };


Should speak "clear".

klown · 2024-01-19T21:37:33Z

src/CommandDelLastEncoding.ts

+  const cellClicked = () => {
+    const newEncoding = [...fullEncoding];
+    newEncoding.pop();
+    setFullEncoding(newEncoding);


As above with ComanndClearEncoding, should speak "delete".

klown · 2024-01-19T21:38:31Z

src/ContentBmwEncoding.test.ts

+  // The aria role is defined
+  const encodingAreaByRole = await screen.findByRole("region");
+  expect(encodingAreaByRole).toBeVisible();
+  expect(encodingAreaByRole).toBeValid();


The display area should have a label, and a test that it's properly set.

Line 37-40 in this test finds the display area by "aria-label", which is to test the area has a properly named label.

Right, I missed that.

klown · 2024-01-19T21:38:56Z

src/ContentBmwEncoding.ts

+  const { id, options } = props;
+  const { columnStart, columnSpan, rowStart, rowSpan } = options;
+
+  // TODO: use a common utility function to calculate the grid position


Use getGridStyle() here since it's available, but see my comment elsewhere about the name of getGridStyle().

klown · 2024-01-19T21:42:14Z

src/GlobalData.ts

@@ -9,14 +9,32 @@
 * https://github.com/inclusive-design/adaptive-palette/blob/main/LICENSE
 */

-"use strict";


Should we add the alwaysStrict compiler flag to ts.config? It has the same effect as "use strict", more or less. I also looked at the stackoverflow article "Use Strict" needed in a typescript file and there are some arguments for making sure the transpiled code has the "use strict"; directive in it, which is part of what alwaysStrict does.

Good point. I'm thinking to set the strict flag to true. This documentation says it enables a wide range of type checking behavior including "alwaysStrict". The linting and running of our code base so far seems working well with strict: true.

What do you think?

Setting strict: true introduces the type verification in the imported modules. Some of them don't have a proper type definitions. I ended up with what you suggested of alwaysStrict: true so that "use strict" is no longer needed in every source file.

Yes, I was worried that strict: true would be too strict. After reading the documentation, I didn't see anything wrong with it, but it looks like you have found that type verification is an issue.

I'm fine with alwaysStrict: true.

klown · 2024-01-19T21:42:46Z

src/GlobalUtils.ts

+ * @param {number} rowSpan - The number of rows that the item will span across.
+ * @return {String} - The grid css.
+ */
+function getGridStyle(columnStart: number, columnSpan: number, rowStart: number, rowSpan: number) {


How about generateGridStyle() or makeGridStyle()? This does not feel like a get() in the sense of retrieving a value. Note that the documentation for the function is "Generate the grid css".

Will change to generateGridStyle().

klown · 2024-01-19T21:43:13Z

src/Palette.test.ts

@@ -77,7 +75,7 @@ describe("Palette component", () => {
  test("Render palette", async() => {

    // render() the palette and then wait until its first cell is available to
-    // insure that the entire palette is in the DOM.
+    // ensure that the entire palette is in the DOM.


Thanks for correcting my grammar :-)

klown · 2024-01-19T21:53:24Z

src/index.d.ts

@@ -11,7 +11,7 @@

 export type BciAvIdType = number | (string|number)[];

-export type OptionsType = {
+export type BlissCellType = {


I'm not sure about "BlissCellType". In the future the cells will contain symbols other than Bliss but they will behave the same and will have similar options. The difference will be the bciAvId property which will be replaced with some other kind of reference to the actual graphic of the symbol. How about "SymbolCellType" as a generic name? Or, perhaps "ActionCellType", since the corresponding components are Action<something>Cell. Or, perhaps it does need to be more specific to allow different kinds of symbols. In that case, how about "BlissSymbolCellType". And if we support, say, the Widgit symbols, they would be "WidgitSymbolCellType". What do you think?

BlissSymbolCellType sounds great. Having Bliss in the name is because this type structure contains a bciAvId field that is specific to Bliss symbols. When we support the graphic symbol or other type of symbols, this field will likely be replaced by the path to the graphic or identifiers to that type of symbol.

klown · 2024-01-19T21:56:53Z

src/keyboards/bmw_palette.json

@@ -1,6 +1,37 @@
 {
    "name": "BMW Palette",
    "cells": {
+        "bmw-encoding-area": {


We should think about making the content display area as a separate JSON and included where necessary. This part of the display contains the encoding area and the delete and clear buttons. I merged your changes into my multi-palette branch, and when I switched away from the BMW palette, the content display area and the "Delete" and "Clear" buttons disappeared. In order to make them render, I had to add their information to the .json files of the other two non-BMW palettes.

This is probably complicated enough to be another issue, and not part of this pull request. What do you think?

Good catch, @klown. We should think about how to make the inclusion of these three elements (the display area, delete and clear buttons) easier.

I agree to address it as a separate issue if needed.

Based on last Tuesday's meeting it looks like this break-out of the display area is complex enough that it warrants a separate issue. I'm fine with leaving things as is in this PR and addressing later. In other words, I don't think this is a blocker against merging this PR.

cindyli · 2024-01-23T21:52:27Z

@klown Thanks for the review and great suggestions. All have been address. Ready for another look.

klown · 2024-01-26T16:33:08Z

A few other possible issues that I have noticed while trying it out.

If the input area is filled with symbols and one keeps adding to it, then an horizontal scroll bar appears and the whole keyboard starts to narrow.
- The horizontal scroll bar might be an issue since it adds another user interaction that AAC users don't want to deal with. I say "might be an issue" since this is a design problem: how do AAC users want the input area to behave? Should there be a scroll bar, or should it expand to a multi-line text box? Or something else? By way of comparison, CBoard behaves the same, adding an horizontal scroll bar when its input area is full.
- The keyboard narrowing needs to be addressed at some point. I don't have any recommendations for fixing it. I don't think it's a blocker, but if you can think of a quick fix, great.
I'm getting a lot of Netlify error messages on FireFox when I run the deployed version. However, I do not see any error messages on Safari. It's possible that it has something to do with my FF settings. A message shows up just below the BMW palette that says "Application error: a client-side exception has occurred (see the browser console for more information)." With Safari, I see a "Deploy Preview" dialog in the same place, with the Netlify logo". Here is a sampling of the browser console, in case it means something to you.

TypeError: document.getElementsByTagName(...)[0] is undefined        vendor.bundle.js:9246:26
DOMException: The operation is insecure.                             bugsnag.js:2457:26
DOMException: The operation is insecure.                             bugsnag.js:2457:15
A client-side exception has occurred, see here for more info: https://nextjs.org/docs/messages/client-side-exception-occurred
                                                                     bugsnag.js:2457:15
DOMException: The operation is insecure.                             bugsnag.js:2457:15
Eight more similar DOMExcptions

…ymbol elements in it

klown · 2024-01-26T15:17:08Z

src/ContentBmwEncoding.test.ts

+  // The aria role is defined
+  const encodingAreaByRole = await screen.findByRole("region");
+  expect(encodingAreaByRole).toBeVisible();
+  expect(encodingAreaByRole).toBeValid();


Right, I missed that.

klown · 2024-01-26T15:18:47Z

src/GlobalData.ts

@@ -9,14 +9,32 @@
 * https://github.com/inclusive-design/adaptive-palette/blob/main/LICENSE
 */

-"use strict";


Yes, I was worried that strict: true would be too strict. After reading the documentation, I didn't see anything wrong with it, but it looks like you have found that type verification is an issue.

I'm fine with alwaysStrict: true.

klown · 2024-01-26T15:21:05Z

src/keyboards/bmw_palette.json

@@ -1,6 +1,37 @@
 {
    "name": "BMW Palette",
    "cells": {
+        "bmw-encoding-area": {


Based on last Tuesday's meeting it looks like this break-out of the display area is complex enough that it warrants a separate issue. I'm fine with leaving things as is in this PR and addressing later. In other words, I don't think this is a blocker against merging this PR.

klown · 2024-01-26T15:52:43Z

src/ContentBmwEncoding.ts

+  const gridStyles = generateGridStyle(columnStart, columnSpan, rowStart, rowSpan);
+
+  return html`
+    <div id="${id}" class="bmwEncodingArea" role="region" aria-label="BMW Encoding Area" style="${gridStyles}">


Somehow my comments about the role="region" were lost in the discussion about using isPresentation for the individual BlissSymbols. I'll repeat and add to it:

The use of role=region is not appropriate. The markup is correct since the region role requires a name or label and a label is provided, but regions are more like sections of text than user interface elements, and are navigation landmarks. Regions are likely to show up in a table of contents, for example.

The content display area is a text field or role textbox and it's a read-only text field as well since there is no insertion cursor, nor direct editing of its contents at this point. In future, if and when the Bliss Unicode effort is complete, there will be the ability to enter Bliss-characters just like one can enter Chinese characters into a text field.

Originally I said, "It is not focus-able, which is correct", but it should be focus-able, since read-only text fields are still focus-able. It's just that they cannot be edited directly.

With respect to making the input area, delete, and clear keys as a share-able palette, the label cannot be "BMW Encoding Area". That's too specific. How about "Bliss Encoding Area", or "Input Area"?

Putting it all together:

<div id="${id}" class="bmwEncodingArea" role="textbox" aria-label="Input Area" aria-readonly="true" style="${gridStyles}"> ...

What do you think?

klown · 2024-01-26T19:36:46Z

src/CommandDelLastEncoding.ts

+  // is to accommodate the component unit test in which the parent palette component is not tested. The
+  // palette state is defined in the palette context.
+  const paletteState = usePaletteState();
+


The comment no longer applies -- there are no separate lines. Did you find a way around having to use separate lines?

cindyli · 2024-01-29T21:05:12Z

@klown thanks for these helpful findings. All addressed and ready for another review.

klown

There are a few small tedious things to fix and once fixed, I can't think of any reason to not merge this.

klown · 2024-01-31T20:03:50Z

src/Palette.ts

@@ -26,7 +26,7 @@ type PalettePropsType = {
 *                 heights and widths of the cells in the palette.
 * @return {Object} - The row and column counts: `{ numRows: ..., numColumns: ...}`.
 */
-function countRowsColumns (paletteDefinition) {
+function countRowsColumns (paletteDefinition: JsonPaletteType) {


You should also change the @param documentation above.

klown · 2024-01-31T20:24:39Z

src/ContentBmwEncoding.scss

@@ -0,0 +1,30 @@
+/*
+ * Copyright 2023 Inclusive Design Research Centre, OCAD University


"Copyright 2023-2024".

Also:

ActionBmwCodeCell.scss,

ActionBmwCodeCell.test.ts

BlissSymbol.test.ts

BlissSymbol.ts

ContentBmwEncoding.scss

Palette.scss

Palette.ts

PaletteStore.test.ts

PaletteStore.ts

SvgUtils.test.ts

index.d.ts

index.js

How's that for picky? ;-)

Being picky in code review is a valuable trait. 👍

cindyli · 2024-01-31T23:14:02Z

@klown new comments have been addressed. Ready for another look. Thanks.

klown · 2024-02-01T20:37:31Z

src/Palette.ts

@@ -22,11 +22,12 @@ type PalettePropsType = {
 * 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.
+ * @param {Object} paletteDefinition - An object in the pre-defined type of


I have a thought. Would it be clearer to say {JsonPaletteType} instead of {Object}?

I admit that I have not been consistent, but I did use {BciAvIdType} as one of the @params and {BlissSVGBuilder} as the return type in SvgUtlis.ts, see the documentation for the getSvgBuilder() function. I did that consistently with SvgUtils.ts, at least. I did not follow that pattern at all in PaletteStore.ts (unfortunately).

We used a similar approach in GPII where we specified @param {Component} to show that the parameter was a fluid component, and a return type of {Promise}, which were actually fluid.promise objects.

What do you think? Even if you agree, I don't think we should use this PR to go around and fix all of the documentation. We could either do it as we edit the files, or make it a separate small task to fix those details.

I see your point and fixed the type here.

klown

Looks good to me, @cindyli!

cindyli added 22 commits October 13, 2023 19:15

feat: initial implementation of BMW encoding area

4a15121

Merge remote-tracking branch 'joseph/feat/create-palette-from-json' i…

71498eb

…nto feat/create-bmw-encoding

feat: add the global message handler

ef00b17

fix: linted

a78a44c

doc: improve code comments

834fa54

doc: add code comments for GlobalMessageHandler

2f415ea

fix: add tests for ContentBmwEncoding and GlobalMessageHandler compon…

c848240

…ents

Merge branch 'feat/add-cellType-registry' into feat/create-bmw-encoding

704bc14

fix: merge missing files in the previous commit

0dbbab5

fix: display proper BlissSymbol in encoding area

28c3577

Merge remote-tracking branch 'joseph/feat/create-palette-from-json' i…

011502c

…nto feat/create-bmw-encoding

fix: adjust style

08c1c71

Merge remote-tracking branch 'joseph/feat/create-palette-from-json' i…

47bb906

…nto feat/create-bmw-encoding

Merge remote-tracking branch 'joseph/feat/create-palette-from-json' i…

df7cb68

…nto feat/create-bmw-encoding

fix: improve css for Bliss symbols and fix tests

b5ddde5

Merge remote-tracking branch 'joseph/feat/create-palette-from-json' i…

2f86063

…nto feat/create-bmw-encoding

test: add tests for showing Bliss symbols in the content area

c94e4aa

fix: add the type check for addBmwCode payload

f079472

Merge branch 'main' into feat/create-bmw-encoding

77e4709

feat: use preact context to coordinate the addition/removal/clearing …

f9f2921

…of BMW codes

feat: add unit tests and integration tests

a48c887

fix: improve styling

d9835ea

cindyli self-assigned this Jan 5, 2024

cindyli requested a review from klown January 5, 2024 17:11

cindyli added 2 commits January 5, 2024 12:53

fix: code clean up

bbe43d3

feat: announce the label of the selected Bliss symbol

bd77ddd

klown requested changes Jan 19, 2024

View reviewed changes

fix: address review comments

5ad860d

cindyli added 5 commits January 29, 2024 09:28

fix: improve css of the grid palette

241b738

fix: the width of the display area no longer changes when appending s…

07aed46

…ymbol elements in it

fix: fix the palette test

e56aba6

fix: the display area now has role="textbox"

4fbcf01

fix: fix the test for ContentBmwEncoding

16c2c36

klown reviewed Jan 29, 2024

View reviewed changes

fix: address more review comments

f14cbaa

klown requested changes Jan 31, 2024

View reviewed changes

fix: improve function comments and copyright messages

1f73831

klown reviewed Feb 1, 2024

View reviewed changes

fix: improve a code comment

a1badc7

klown approved these changes Feb 1, 2024

View reviewed changes

cindyli merged commit 598529a into inclusive-design:main Feb 1, 2024
8 checks passed

cindyli deleted the feat/create-bmw-encoding branch February 1, 2024 21:08

cindyli mentioned this pull request Feb 1, 2024

Implement a content display area and corresponding buttons that add/remove/clear contents in it #3

Closed

		@@ -0,0 +1,30 @@
		/*
		* Copyright 2023 Inclusive Design Research Centre, OCAD University

feat: implement a content display area and associated buttons (resolves #2) #4

feat: implement a content display area and associated buttons (resolves #2) #4

Conversation

cindyli commented Jan 5, 2024

netlify bot commented Jan 5, 2024 • edited

✅ Deploy Preview for adaptive-palette ready!

klown left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cindyli commented Jan 23, 2024

klown commented Jan 26, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cindyli commented Jan 29, 2024

klown left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cindyli commented Jan 31, 2024

klown Feb 1, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

klown left a comment

Choose a reason for hiding this comment

netlify bot commented Jan 5, 2024 •

edited

klown commented Jan 26, 2024 •

edited

klown Feb 1, 2024 •

edited