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

Conversation

klown
Copy link
Contributor

@klown klown commented Oct 2, 2023

Description

WIP

This draft pull request uses a JSON file to render a BMW palette of encoding symbols as keys. It defines Preact components for:

  • PaletteCells that are interactive and behave as keys on an onscreen keyboard,
  • Palette of such cells, arranged according to the row/column information in the JSON file.

Related issues

Resolves #1

index.html Outdated
<meta charset="UTF-8" />
<meta name="viewport" content="width=device-width, initial-scale=1.0" />
<meta name="color-scheme" content="light dark" />
<link href="./src/PaletteCell.css" rel="stylesheet"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. I think it's agreed to use scss rather than css for styling. Scss is helpful in writing cleaner and more structural code.

  2. The styling file doesn't need to be included in the html file. It can be directly imported into its component file. For example, to have PaletteCell.css take effect, in the PaletteCell.ts, write import "./PaletteCell.scss" in the import section.

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, and good to know. I'll switch the css over to scss and import into the .ts file.

* https://github.com/inclusive-design/adaptive-palette/blob/main/LICENSE
*/

.paletteCell {
Copy link
Contributor

Choose a reason for hiding this comment

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

Using scss, line 12-28 can turn into a scoped nested structure:

.paletteCell {
  ...

  &:hover, &:focus {
    ...
  }

  & > svg {
    ...
  }
}

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 suggestion. I know little about scss nor how it works. Time to learn :-)

src/Palette.ts Outdated
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.

src/Palette.ts Outdated
cellIds.forEach((id) => {
const aCell = paletteDefinition.cells[id];
const paletteCell = html`
<${PaletteCell} id="${id}" labelText="${aCell.label}"
Copy link
Contributor

Choose a reason for hiding this comment

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

A palette cell can be one of types: ActionBmwIcon, ContentBmwEncoding etc.

I was thinking the type field defined for each cell specifies what type it is, and a component named in this type name can be instantiated to render this cell. If we use this approach, PaletteCell component should be renamed to ActionBmwIcon (let me know if you come up with a better name), and the aCell.type value should replace ${PaletteCell} in this line.

I'm also thinking each cell type has an options block in the JSON for holding the cell specific information. This options can be directly when this cell gets instantiated so the instantiator at this line doen't need to worry about what property values are expected. The property structure can be verified inside the cell component. If we do so, the JSON structure for ActionBmwIcon will look like:

{
    "type": "ActionBmwKey",
    "rowStart": 4,
    "rowSpan": 1,
    "columnStart": 1,
    "columnSpan": 1,
    "options": {
        "label": "ADJ.+ER",
        "BCI-AV-ID": 24879
    }
}

Let me know what you think. Thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A palette cell can be one of types: ActionBmwIcon, ContentBmwEncoding etc.

And also "Branch" and "BranchBack" for navigating among different layers. And others we haven't thought of yet.

I was thinking the type field defined for each cell specifies what type it is,
and a component named in this type name can be instantiated to render this
cell. If we use this approach, PaletteCell component should be renamed to
ActionBmwIcon (let me know if you come up with a better name), and the
aCell.type value should replace ${PaletteCell} in this line.

I have been thinking about this, and I keep coming back to using classes since that is a technique for putting common aspects of all palette cells in a base component and deriving more specialized cases therefrom. I'm not sure how to do that with Functional Preact components, but perhaps there is a way. Maybe it's as simple as calling "base" Functions from "derived" Functions. And, in that case, maybe Function PaletteCell() is the most basic in the chain.

Another thought I had about the type field was to put it into a data-cellType attribute in the html itself since all of the action cells are button elements. Without the data-cellType attribute, it is impossible to tell what kind of button it is (unless the class attribute has sufficient information?). My intuition is that "Undo" or "Delete" are different types of actions than entering a BMW code. But, I don't know yet if there is any use to noting the cell type in the markup. I'll file away the idea for now.

I'm also thinking each cell type has an options block in the JSON for holding
the cell specific information. This options can be directly when this cell
gets instantiated so ...

There is a missing verb here: "... can be directly " ? Copied? I think you mean something like this in terms of using the options block: <$PaletteCell options=${aCell.options} id=${id} ....

Then the PaletteCell component, or sub-component, uses only the options that are appropriate for that type.

That's a great idea. In that case, I am inclined to put the row/column info in the options block as well.

@cindyli
Copy link
Contributor

cindyli commented Oct 3, 2023

@klown you read my mind and captured exactly what I missed in "can be directly ...". 👍

I also thought about if a base cell component should be created. The only common information I can think of for now that can be shared by all cells is the row/column information. But I was debating if having a base component handling this info would make it easier or harder to extend from it as the base component doesn't render anything. A better way might be to create a common shared utility component that can be imported when needed, which is a composition rather than an inheritance. What do you think?

This is also why I didn't include row/col values into options. Thinking about what you said, it seems making sense to move row/col into options as they are cell information and it saves from writing them repeatedly as component props. I will regenerate the BMW palette JSON file with this change.

@cindyli
Copy link
Contributor

cindyli commented Oct 3, 2023

@klown the new JSON with the options block is generated here. Note that the filename has been changed to bmw_palette.json.

@klown
Copy link
Contributor Author

klown commented Oct 5, 2023

@cindyli wrote:

I also thought about if a base cell component should be created. The only common information I can think of for now that can be shared by all cells is the row/column information. But I was debating if having a base component handling this info would make it easier or harder to extend from it as the base component doesn't render anything. A better way might be to create a common shared utility component that can be imported when needed, which is a composition rather than an inheritance. What do you think?

I now think that it's too early to tell what all the cells have in common that constitute a useful base. For now, I think your earlier suggestion of using the type property of a cell to instantiate the appropriate component is a good one and I will try to implement it. As you know from the discussion in Element, this is not as easy as just doing: <${cell.type} ... because cell.type is a string and Preact wants a component function here. We need to explore a way of mapping the type string to the named component.

Then, as we define the components for the palette, if we find a common base, we can do something to handle those common features (I like the idea of using composition to do it, btw).

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.

src/Palette.ts Outdated
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.

index.html Outdated
</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.

Comment on lines +23 to +31
const singleBciAvId = {
bciAvId: 12335,
label: "VERB"
};

const arrayBciAvId = {
bciAvId: [12335, "/", 8499],
label: "VERB+S"
};
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be clearer to split this test into two sections or two tests, one for testing the number bciAvId and the other for testing the array bciAvId.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, done.

## How to render a palette

`Palette.ts` constructs a palette based on a JSON file that contains a list
of the cells in the palette. An example is found in
Copy link
Contributor

Choose a reason for hiding this comment

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

"An example is found ..." -> "An example can be found ..."

Copy link
Contributor Author

@klown klown Dec 1, 2023

Choose a reason for hiding this comment

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

The active voice is preferable to the passive. The use of "can be" is passive. It might be better to say, "An example is available ...".

render this cell. The `options` contains information to be passed to the
component.

When a new `type` value is introduced, developers need to:
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it clearer to add a h2 title "How to add a new cell type" to the section starting from this line to the end of the document?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay.

<body>
<h2>Palette Based on JSON</h1>
<div id="bmwKeyCodes">
<script type="module">
Copy link
Contributor

Choose a reason for hiding this comment

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

What do think about the idea that moves the javascript in this <script> tag to index.js that is then loaded into this page?

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've moved the code.

package.json Outdated
@@ -8,13 +8,15 @@
"preview": "vite preview",
"lint": "run-p lint:*",
"lint:js": "eslint .",
"lint:markdown": "markdownlint-cli2 \"./**/*.md\" \"#node_modules\" \"#dist\"",
"lint:markdown": "markdownlint-cli2 \"./**/*.md\" \"#node_modules\" \"#dist\" \"#Bliss-Blissary-BCI-ID-Map\"",
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the linting exclusion on #Bliss-Blissary-BCI-ID-Map still needed? It doesn't seem this directory is eventually created in this pull request.

Copy link
Contributor Author

@klown klown Dec 1, 2023

Choose a reason for hiding this comment

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

That was needed when the Blissary Map was a sub-module. Since we now use fetch() to get the map, the sub-module is gone. So I have removed the lint exclusion rule.

* https://github.com/inclusive-design/adaptive-palette/blob/main/LICENSE
*/

import { fetch } from "whatwg-fetch";
Copy link
Contributor

Choose a reason for hiding this comment

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

If you happen to have the solution article about using this npm package to fix the use of fetch() in the Jest, can you add a code comment to explain what this script is for as well as including the article? Thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. I have added a comment/link.

* @param: {Palette}.name - The internal name of the palette.
* @param: {String} name - Optional, the preferred name of the palette.
*/
addPalette (palette, paletteName?: string) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a type check for palette parameter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That requires a new type. The only property that the PaletteStore uses is the name of the palette, for lookup. A simple approach for a type then is:

type Palette = {
  name: string,
  palette: any
}

The problem is that lint will complain about the any. Are we sure of the palette's structure? If so, there are additional types to replace the any:

type Palette = {
  name: string,
  palette: JsonPalette  // In future, e.g., JsonPalette | OtherPalette | ...
}

type JsonPalette = {
  name: string,
  cells?: Cell
}

type Cell {
  type: string,
  options: Options,
}

type Options {
  label: string,
  bciAvId: number | (string|number)[],
  rowStart: number,
  rowSpan: number,
  columnStart: number,
  columnSpan: number
}

Note: I'm not sure how to actually specify the cells field in the JsonPalette type. It's a variable number of Cells, but it is not an array.

What do think about the two approaches? Should we use the complete one?

Copy link
Contributor

Choose a reason for hiding this comment

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

It might be time to create a src/index.d.ts to export all shared types. Its content looks like:

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

export type ActionBmwCodeCellType = {
  type: string,
  options: {
    label: string,
    columnStart: number,
    columnSpan: number,
    rowStart: number,
    rowSpan: number,
    bciAvId: BciAvId
  }
};

export type ContentBmwEncodingType = {
  id: string,
  columnSpan: number,
  options: {
    columnStart: number,
    rowStart: number,
    rowSpan: number
  }
};

export type PaletteType = {
  name: string,
  cells: {
    [key: string]: ActionBmwCodeCellType | ContentBmwEncodingType
  }
};

I'm using your BciAvId type definition and importing from BlissSymbol.ts. This kind of shared types belong to a shared file. This shared file (src/index.d.ts) will also help others to understand our base data structures. For types that are only used by individual components, they can continue to be private in the component.

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 very much like the idea of a common place for shared types; very nice.

I had thought that this technique was not allowed because we were keeping code, styles, etc. grouped by the name of the component. For example, there is a collection of ActionBmwCodeCell.ts, ActionBmwCodeCell.scss, etc. files for everything that has to do with ActionBmwCodeCells. That made me wonder how to share the types (and interfaces if we ever start to use them).

Breaking out the shared types into a single file makes total sense. BTW, Where did the name index.d.ts come from? Is that the practice with TypeScript?

Copy link
Contributor

Choose a reason for hiding this comment

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

I've been reading the Preact repo to learn how it organizes the code base. index.d.ts is what I learnt there - https://github.com/preactjs/preact/blob/main/src/index.d.ts. Then I searched on index.d.ts and found a typescript doc about *.d.ts - https://www.typescriptlang.org/docs/handbook/declaration-files/templates/module-d-ts.html

* @param: {String} paletteName - The palette to remove.
* @return {Palette} reference to the removed palette.
*/
removePalette (paletteName) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a type check for paletteName. Same comment for line 93.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it is a string type.

src/SvgUtils.ts Outdated
catch (err) {
console.error(err);
console.error(`GETSVGMARKUPSTRING(): using question mark for SVG builder argument from bci-av-id = ${bciAvId}`);
builder = new BlissSVGBuilder(DEFAULT_SVG_MARKUP_STRING); // question mark
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm thinking when an error occurs and the SVG builder doesn't return a SVG, would it make more sense to return undefined instead of a SVG of a question mark? Returning undefined can be captured by the caller component to deal as an exception. Returning a question mark seems like a silent fail that the caller would assume things went well and a question mark is the expected.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay. Another approach is to not catch the error and let it continue up the stack.

Regardless, the BlissSymbol component will need to handle the thrown error or the undefined return value. It could (1) not render any SVG and leave that part of the cell blank, or (2) render some other SVG that means "unknown symbol".

For the second approach either another call to the SVG builder will be necessary or there is a constant SVG defined for this purpose. Either way this is a design issue as to what the best way to communicate "unknown" or "unavailable symbol" -- a blank cell, or something that means "unknown/unavailable".

What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree this is a design issue. At this development stage, I lean towards the option 1 that does not render any SVG, as well as reporting a console.error() with what's happening so developers realize this BCI-AV-ID should be checked.

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. Note that when there is the error, no <svg> element is added, not even an empty one.

// `bciAvIdArray` is also from the BMW json file using the codes for
// "VERB+EN". The `expectedX` constants are based on a manual lookup of the
// blissary ids.
const singleBciAvId = 23409; // CONJ.
Copy link
Contributor

Choose a reason for hiding this comment

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

A more structured way to organize test cases from line 22-26 could be:

const testCases = {
  "numberInput": {
    "input": 23409,
    "expected": "B823"
  },
  "arrayInput": {
    "input": [ 12335, "/", 8499 ],
    "expected": "B106/B12"
  },
  "nonexistentBciAvId": {
    "input": 1,
    "expected": undefined
  }
  ...
}

The test then loop through testCases with the same code to handle each input and check the expected.

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 tried the loop, but have not committed the code. I found it more difficult to see what is being tested. I prefer having readable explicit tests to having to think about the parameters in a loop where the tests are executed, and then look up their values in a different block of code. The loop technique feels opaque to me.

Copy link
Contributor

Choose a reason for hiding this comment

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

Understand. Please go ahead with the approach that works best for you.

}
});
}
return finalCode;
Copy link
Contributor

Choose a reason for hiding this comment

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

When the corresponding Bliss ID is not found for a BCI-AV-ID, it might be better to console.erro() the error so the developer knows which BCI-AV-ID is problematic.

Copy link
Contributor Author

@klown klown Dec 1, 2023

Choose a reason for hiding this comment

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

There is a console.error() where the exception occurs, inside getSvgString() (and also in the new getSvgElement()).

This might be related: I don't think bciToBlissaryId() should be exported, since it is only used within the SvgUtils module. The reason that is was exported it was solely for the purpose of writing tests for it. I have since looked into testing "private" functions and there is more about that in my response above about the addition of tests for the Palette component.

If bciToBlissaryId() is private, then the only place where the throw will occur is in the two getSvg...() functions, where the console.error() logs the problem

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it, and great to know the error is already reported.

@klown klown marked this pull request as ready for review November 28, 2023 14:34
Copy link

netlify bot commented Nov 28, 2023

Deploy Preview for adaptive-palette ready!

Name Link
🔨 Latest commit 352ce6c
🔍 Latest deploy log https://app.netlify.com/sites/adaptive-palette/deploys/6583123d4a09750008e90e4f
😎 Deploy Preview https://deploy-preview-2--adaptive-palette.netlify.app
📱 Preview on mobile
Toggle QR Code...

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
Copy link
Contributor Author

klown commented Dec 1, 2023

Hi @cindyli . I've addressed most of your review. For the rest I have made comments. Please give everything another review. Thanks!

Comment on lines 30 to 31
const { columnStart, columnSpan, rowStart, rowSpan } = props.options;
const { bciAvId, label } = props.options;
Copy link
Contributor

Choose a reason for hiding this comment

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

These two lines can be combined into one line as they are all parsing props.options.

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 reason they were split is that putting them all on one line is too long. I've found a way to split a single destructuring statement across the two lines.

import { html } from "htm/preact";
import { getSvgElement } from "./SvgUtils";

export type BciAvId = number | (string|number)[];
Copy link
Contributor

Choose a reason for hiding this comment

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

For type variables, what about adding a suffix "Type" as a naming convention so it's clear this variable is a type definition for the corresponding value. "BciAvId" -> "BciAvIdType".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea @cindyli. I added an index.d.ts for defining shared types. There are two kinds: types that are used in a variety of contexts (e.g., BciAvIdType) and others that were specific to the props parameter of the Functional Component. In that case, I used the ending "PropsType", e.g., PalettePropsType as a way of making it explicit that these types are used for the properties for the rendering function.

I did not include the ContentBmwEncodingPropsType since it is not part of this pull request. And, I noticed that the way the options are specified in the JSON palette description files is not quite the same as is used in the ContentBmwEncodingPropsType. For now, I've defined an OptionsType that works with the JSON palette files and with the ActionBmwCodeCellPropsType. However, certain fields in the OptionsType could be marked as optional in order that the same type definition work with JsonPaletteType, ActionBmwCodeCellPropsType, and ContentBmwEncodingPropsType, and other cell types we might have in the future. The grid coordinates are a likely candidate for this.

But, take a look, and let me know what you think.

src/index.d.ts Outdated
Comment on lines 35 to 42
export type PalettePropsType = {
json: JsonPaletteType
};

export type ActionBmwCodeCellPropsType = {
id: string,
options: OptionsType
};
Copy link
Contributor

Choose a reason for hiding this comment

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

These component specific type definitions can be defined in the component as a private type. What do you think?

Copy link
Contributor Author

@klown klown Dec 20, 2023

Choose a reason for hiding this comment

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

I just searched the code base, and you are correct. The only place that ActionBmwCodeCellPropsType is used is inside ActionBmwCodeCell.ts. I'll move that one out of the shared index file and make it local, and do the same with PalettePropsType.

Thanks for noticing that!

Copy link
Contributor

Choose a reason for hiding this comment

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

Same with PalettePropsType. It's only used in Palette.ts.

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 also found that I missed a BlissSymbolProps type in terms of the new naming convention. I fixed that too.

@cindyli cindyli merged commit 2132549 into inclusive-design:main Dec 20, 2023
8 checks passed
cindyli added a commit that referenced this pull request Feb 1, 2024
feat: implement a content display area and associated buttons (resolves #2)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Develop Preact components for the palette cells/keys and render them in a grid based on a JSON file
2 participants