Storybook components

[rolfheij-sil]

This work adds 5 components, which are exposed to the PAPI, and can be explored using storybook.
Resolves #39 & Resolves #40

There is a known issue with the ComboBox component. The signature for its onChange event is not correct (see ComboBox.tsx line 38), and triggering its onFocus event (and probably others) in Storybook produces an error.

---

This change is 

[irahopkinson]

Wow, thanks for all your good work on this both of you. I have some questions and requested changes. Do note that not all my comments are blocking (check the icon top-right for each comment).

Reviewed 13 of 30 files at r1, 9 of 22 files at r2, 21 of 21 files at r4, all commit messages.
Reviewable status: all files reviewed, 22 unresolved discussions (waiting on @rolfheij-sil and @tjcouch-sil)

---

.babelrc.json line 16 at r4 (raw file):

  ],
  "plugins": []
}

Blank line at end of file is missing (note the red symbols).

---

package.json line 66 at r4 (raw file):

    "start:data": "dotnet watch --project c-sharp/ParanextDataProvider.csproj",
    "test": "jest",
    "storybook": "storybook dev -p 6006",

When I run this I get the following error (truncated at page boundary:

---

package.json line 67 at r4 (raw file):

    "test": "jest",
    "storybook": "storybook dev -p 6006",
    "build-storybook": "storybook build"

To follow the established pattern rename this script storybook:build.

---

package.json line 67 at r4 (raw file):

    "test": "jest",
    "storybook": "storybook dev -p 6006",
    "build-storybook": "storybook build"

When I run this I get the following error:

---

package.json line 108 at r4 (raw file):

    "@pmmmwh/react-refresh-webpack-plugin": "^0.5.10",
    "@svgr/webpack": "^6.5.1",
    "@teamsupercell/typings-for-css-modules-loader": "^2.5.2",

Keep the alphabetical order of items. npm normally does this for you, but perhaps this occured due to a merge. Move this line and the one above to below @storybook/... items.

---

package.json line 170 at r4 (raw file):

    "sass": "^1.59.3",
    "sass-loader": "^13.2.0",
    "storybook": "^7.0.0-beta.60",

We don't tend to use beta versions in production. Just checking in on the reasons for using this at a beta version?

---

.vscode/launch.json line 63 at r4 (raw file):

      }
    },
      },

This is in the wrong place. VSCode can't launch anything at the moment. Move it to to below the storybook item (just before the next square bracket).

---

src/renderer/components/papi-components/Button.tsx line 32 at r4 (raw file):

 */
function Button({
  primary = false,

See our guidelines on naming booleans. Change to something like isPrimary. However if this is following a React naming convention then feel free to ignore this. E.g. I'm not asking you to rename disabled because that is a well known HTML attribute name even though it doesn't follow our naming guidelines (they are just guidelines, not rules).

Code quote:

primary

---

src/renderer/components/papi-components/ComboBox.tsx line 21 at r4 (raw file):

   * @default false
   */
  error?: boolean;

See my comment above about our naming guidelines. Change this to isError or hasError.

---

src/renderer/components/papi-components/ComboBox.tsx line 26 at r4 (raw file):

   * @default false
   */
  fullWidth?: boolean;

We could debate this one but following our naming guidelines here would mean users of this component could also follow the guidelines. So you could change this to isFullWidth.

---

src/renderer/components/papi-components/ComboBox.tsx line 39 at r4 (raw file):

   */
  onChange?: () => void; // This event handler does not have the suggested signature,
  // but using the suggested one leads to additional MUI import, and I'm not sure if that's a great idea

What import does it add?

---

src/renderer/components/papi-components/Slider.tsx line 40 at r4 (raw file):

   * @default false
   */
  marks?: boolean;

See my comment above about our naming guidelines. Another one that could be debated so you could change this to hasMarks.

---

src/renderer/components/papi-components/Switch.tsx line 9 at r4 (raw file):

   * If `true`, the component is checked.
   */
  checked?: boolean;

See my comment above about our naming guidelines. Changing in this file to isChecked, isPrimary, hasError.

checked is debatable since that is a known HTML attribute, but since this is a switch rather than a checkbox I might argue for calling it isOn.

---

src/renderer/components/papi-components/Textfield.tsx line 19 at r4 (raw file):

   * @default false
   */
  error?: boolean;

See my comment above about our naming guidelines. Changing in this file to hasError, isFullWidth, isRequired .

required is debatable since that is a known HTML attribute.

---

src/stories/Button.stories.tsx line 1 at r4 (raw file):

import type { Meta, StoryObj } from '@storybook/react';

I haven't confirmed these stories since I couldn't get storybook to build (see comment above). So I'm assuming they are working.

---

src/stories/Button.stories.tsx line 14 at r4 (raw file):

  },
};

Nit: you could remove the whitespace here since the export below goes with the const above.

---

src/stories/Button.stories.tsx line 16 at r4 (raw file):

export default meta;
type Story = StoryObj<typeof Button>;

Add white space above this line, i.e. add a line feed.

---

src/stories/ComboBox.stories.tsx line 19 at r4 (raw file):

export default meta;
type Story = StoryObj<typeof ComboBox>;

See the 2 whitespace comments in src/stories/Button.stories.tsx.

---

src/stories/Slider.stories.ts line 36 at r4 (raw file):

export default meta;
type Story = StoryObj<typeof Slider>;

See the 2 whitespace comments in src/stories/Button.stories.tsx

---

src/stories/Switch.stories.ts line 17 at r4 (raw file):

export default meta;
type Story = StoryObj<typeof Switch>;

See the 2 whitespace comments in src/stories/Button.stories.tsx

---

src/stories/Textfield.stories.ts line 29 at r4 (raw file):

export default meta;
type Story = StoryObj<typeof Textfield>;

See the 2 whitespace comments in src/stories/Button.stories.tsx

---

src/stories/assets/code-brackets.svg line 0 at r4 (raw file):
Where were all these SVG files taken from? Do we have rights to use them and what is their license? That should be documented somewhere - like add a README.md file to the folder.

---

src/renderer/components/papi-components/button.css line 11 at r4 (raw file):

}
.papi-button.primary {
  color: white;

Should all these colors be using theme variables so we are consistent across components? I guess we might need SCSS or LESS for that so that we can import theme variables? Perhaps we need to add a separate issue for this.

---

src/renderer/components/papi-components/textfield.css line 1 at r4 (raw file):

/* These rules don't do anything */

¿qué? You might need to explain why they are included if they "don't do anything", or just remove them.

[rolfheij-sil]

Reviewable status: all files reviewed, 21 unresolved discussions (waiting on @irahopkinson, @katherinejensen00, and @tjcouch-sil)

---

.babelrc.json line 16 at r4 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

Blank line at end of file is missing (note the red symbols).

Fixed

---

package.json line 66 at r4 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

When I run this I get the following error (truncated at page boundary:

I think this is a known issue that has something to do with Webpack.
It is possible to run storybook like this:
npm run storybook -- --debug-webpack

@tjcouch-sil Can you tell us / Ira more about why we're running it like this?

---

package.json line 67 at r4 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

To follow the established pattern rename this script storybook:build.

I think it should be build:storybook?

---

package.json line 67 at r4 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

When I run this I get the following error:

I get a different error when trying this build step. See below. I'm not sure if we even need this build step. @tjcouch-sil Do we need this?

---

package.json line 108 at r4 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

Keep the alphabetical order of items. npm normally does this for you, but perhaps this occured due to a merge. Move this line and the one above to below @storybook/... items.

Done.

---

package.json line 170 at r4 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

We don't tend to use beta versions in production. Just checking in on the reasons for using this at a beta version?

@tjcouch-sil @katherinejensen00 Can you answer this one?

---

.vscode/launch.json line 63 at r4 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

This is in the wrong place. VSCode can't launch anything at the moment. Move it to to below the storybook item (just before the next square bracket).

Done.

---

src/renderer/components/papi-components/Button.tsx line 32 at r4 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

See our guidelines on naming booleans. Change to something like isPrimary. However if this is following a React naming convention then feel free to ignore this. E.g. I'm not asking you to rename disabled because that is a well known HTML attribute name even though it doesn't follow our naming guidelines (they are just guidelines, not rules).

I've updated the names of the booleans that do not have a direct HTML/MUI counterpart

---

src/renderer/components/papi-components/ComboBox.tsx line 21 at r4 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

See my comment above about our naming guidelines. Change this to isError or hasError.

Done.

---

src/renderer/components/papi-components/ComboBox.tsx line 26 at r4 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

We could debate this one but following our naming guidelines here would mean users of this component could also follow the guidelines. So you could change this to isFullWidth.

fullWidth is a prop on the MUI component, that we're exposing to the user of our combo box. Of course we can expose it as isFullWidth, but that might be confusing. Perhaps we should discuss conventions for these kinds of situations with the team?

---

src/renderer/components/papi-components/ComboBox.tsx line 39 at r4 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

What import does it add?

AutocompleteChangeDetails and AutocompleteChangeReason
Both are directly related to the Autocomplete component (which we've rebranded as Combobox), so maybe it's not a problem after all

---

src/renderer/components/papi-components/Slider.tsx line 40 at r4 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

See my comment above about our naming guidelines. Another one that could be debated so you could change this to hasMarks.

This is also a MUI prop that we're exposing (see my comment on (is)fullWidth on ComboBox.tsx)

---

src/renderer/components/papi-components/Switch.tsx line 9 at r4 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

See my comment above about our naming guidelines. Changing in this file to isChecked, isPrimary, hasError.

checked is debatable since that is a known HTML attribute, but since this is a switch rather than a checkbox I might argue for calling it isOn.

This is also a MUI prop that we're exposing (see my comment on (is)fullWidth on ComboBox.tsx)

---

src/renderer/components/papi-components/Textfield.tsx line 19 at r4 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

See my comment above about our naming guidelines. Changing in this file to hasError, isFullWidth, isRequired .

required is debatable since that is a known HTML attribute.

These are also MUI props that we're exposing (see my comment on (is)fullWidth on ComboBox.tsx)

---

src/stories/Button.stories.tsx line 1 at r4 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

I haven't confirmed these stories since I couldn't get storybook to build (see comment above). So I'm assuming they are working.

As mentioned earlier, you can run with npm run storybook -- --debug-webpack

---

src/stories/Button.stories.tsx line 14 at r4 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

Nit: you could remove the whitespace here since the export below goes with the const above.

Done

---

src/stories/Button.stories.tsx line 16 at r4 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

Add white space above this line, i.e. add a line feed.

Done.

---

src/stories/ComboBox.stories.tsx line 19 at r4 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

See the 2 whitespace comments in src/stories/Button.stories.tsx.

Done.

---

src/stories/Slider.stories.ts line 36 at r4 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

See the 2 whitespace comments in src/stories/Button.stories.tsx

Done.

---

src/stories/Switch.stories.ts line 17 at r4 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

See the 2 whitespace comments in src/stories/Button.stories.tsx

Done.

---

src/stories/Textfield.stories.ts line 29 at r4 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

See the 2 whitespace comments in src/stories/Button.stories.tsx

Done. At least we were consistent 😉

---

src/stories/assets/code-brackets.svg line at r4 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

Where were all these SVG files taken from? Do we have rights to use them and what is their license? That should be documented somewhere - like add a README.md file to the folder.

@katherinejensen00 Can you answer this one?

---

src/renderer/components/papi-components/button.css line 11 at r4 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

Should all these colors be using theme variables so we are consistent across components? I guess we might need SCSS or LESS for that so that we can import theme variables? Perhaps we need to add a separate issue for this.

There's still a lot of unknowns regarding how we will be using CSS, so the .css files in this review are nothing more than a proof of concept to show that we can stylize MUI components

---

src/renderer/components/papi-components/textfield.css line 1 at r4 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

¿qué? You might need to explain why they are included if they "don't do anything", or just remove them.

Sorry, I get that this is weird 😅 As mentioned earlier, there's still a lot of unknowns regarding how we will be using CSS, so the .css files in this review are nothing more than a proof of concept to show that we can stylize MUI components (and in this case the CSS doesn't even do anything yet)

[rolfheij-sil]

Thank you! And thanks for reviewing that much code this quick.

Reviewable status: 32 of 43 files reviewed, 21 unresolved discussions (waiting on @irahopkinson, @katherinejensen00, and @tjcouch-sil)

[rolfheij-sil]

Reviewable status: 30 of 45 files reviewed, 22 unresolved discussions (waiting on @irahopkinson, @katherinejensen00, and @tjcouch-sil)

---

src/renderer/components/papi-components/ComboBox.tsx line 82 at r6 (raw file):

Previously, rolfheij-sil wrote…

I'll make the combobox use the MUI Textfield instead of our own and pass down props explicitly

I've tried to improve this but I don't think it's perfect yet. Perhaps we can have a look at it together

[irahopkinson]

Reviewed 14 of 15 files at r8, 1 of 1 files at r9, all commit messages.
Reviewable status: all files reviewed, 23 unresolved discussions (waiting on @katherinejensen00, @rolfheij-sil, and @tjcouch-sil)

---

.eslintrc.js line 2 at r6 (raw file):

Previously, rolfheij-sil wrote…

Done.

This would be better like this:

  extends: [
     // https://github.com/electron-react-boilerplate/eslint-config-erb/blob/main/index.js
    // airbnb rules are embedded in erb https://github.com/airbnb/javascript/tree/master/packages/eslint-config-airbnb
    'erb',
    // https://github.com/storybookjs/eslint-plugin-storybook/blob/main/lib/configs/recommended.ts
    'plugin:storybook/recommended'
  ],

---

package.json line 67 at r4 (raw file):

Previously, rolfheij-sil wrote…

I think @katherinejensen00 got this same error. We're you able to resolve it Katherine?

I'm wondering if you get this to build you will solve the ComboBox error, or when you solve the ComboBox error this might build correctly. However if no one knows what it does or even uses it then take it out until its needed.

---

src/renderer/components/papi-components/Button.tsx line 34 at r9 (raw file):

function Button({
  isPrimary = false,
  isDisabled: disabled,

This is more complicated to destructure this here and is not consistent with how its done in other components. Just use isDisabled below instead of destructuring here.

---

src/renderer/components/papi-components/ComboBox.tsx line 45 at r6 (raw file):

Previously, rolfheij-sil wrote…

I'll go with Ira's idea for now. I guess we will come back to this discussion later

Drop the Mui prefix from both of these so Mui doesn't leak out to users of this component.

---

src/renderer/components/papi-components/Switch.tsx line 36 at r5 (raw file):

   * Optional click handler
   */
  onClick?: MouseEventHandler<HTMLButtonElement>;

Did I miss a discussion? I'm surprised to see this removed since onClick is the most used thing for any component.

---

src/stories/Textfield.stories.ts line 1 at r7 (raw file):

Previously, rolfheij-sil wrote…

Done

Er... you still need to change the filename. Note my post above about how to do that in Windows rather than Git.

---

src/renderer/components/papi-components/Textfield.tsx line 60 at r7 (raw file):

Previously, rolfheij-sil wrote…

Done

This is not a Git problem but a Windows problem. So you don't need to add Git commits to fix it. I usually rename the file by adding a character, e.g. from Texfield.ts to Text-field.ts, then removing and changing case at the same time, i.e. to TextField.ts. If you go directly from Texfield.ts to TextField.ts Windows changes the name you see but not the underlying name on the filesystem so Git sees no change but we do see the change and assume Git has the issue. I'm also post this in Discord for others to note.

[tjcouch-sil]

Thanks for the great updates!! Keep up the good work. Looking forward to more soon!

Reviewed 14 of 15 files at r8, 1 of 1 files at r9, all commit messages.
Reviewable status: all files reviewed, 14 unresolved discussions (waiting on @irahopkinson, @katherinejensen00, and @rolfheij-sil)

---

src/renderer/components/papi-components/Button.tsx line 34 at r9 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

This is more complicated to destructure this here and is not consistent with how its done in other components. Just use isDisabled below instead of destructuring here.

FYI this happens when you have an object being destructured and then rename one of the members of that object type. I guess TypeScript thinks it's being helpful by not renaming the variable created by the destructure. So you just kinda have to keep an eye out for this kind of thing when renaming type members. An easy tweak to fix this is to rename disabled to isDisabled on this line as well. Then it will have isDisabled: isDisabled on this line (and will use isDisabled below), so you can shorten it to just isDisabled

---

src/renderer/components/papi-components/ComboBox.tsx line 25 at r6 (raw file):

Previously, rolfheij-sil wrote…

The MUI Textfield component has an error prop that we've exposed. I think all it does is change the styling of the Textfield to show the user that something is wrong with the input. Since the Combobox uses the Textfield, I thought it might be a good idea to add the prop here too

Gotcha. For some reason, I thought there was also a separate error prop of some sort that displayed the error string passed in to the user. If that doesn't exist, this is fine. Thanks!

---

src/renderer/components/papi-components/ComboBox.tsx line 34 at r6 (raw file):

Previously, rolfheij-sil wrote…

Good question, I don't know how it gets translated. Any idea what type we should use? String[] should work, I guess?

https://mui.com/material-ui/react-autocomplete/#options-structure
Looks like they expect an array of objects that have label: string if you don't override their getOptionLabel. As such, I would be in favor of making ComboBox generic as I mention in a thread below. We can make it something like type MyOptionType = string | { label: string } | Record<string, unknown> and ComboBox<T extends MyOptionType>. Then we can explain that they need to put a label on each option unless they override getOptionLabel. Or we can not give them the option to override getOptionLabel right now and just tell them they have to provide a label like type MyOptionType = string | { label: string } - that might actually be a cleaner way for now :)

---

src/renderer/components/papi-components/ComboBox.tsx line 45 at r6 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

Drop the Mui prefix from both of these so Mui doesn't leak out to users of this component.

I believe Ira was suggesting we declare new types that are simple aliases over the existing types. Like at the top of this file you would want to write export type ComboBoxChangeDetails = MuiComboBoxChangeDetails;, then you can use ComboBoxChangeDetails here. That way, it is our own type, and we can do what we want with it.

---

src/renderer/components/papi-components/ComboBox.tsx line 82 at r6 (raw file):

Previously, rolfheij-sil wrote…

I've tried to improve this but I don't think it's perfect yet. Perhaps we can have a look at it together

Nah, this is great :) thanks!

---

src/stories/ComboBox.stories.tsx line 13 at r9 (raw file):

    hasError: { control: 'boolean' },
    isFullWidth: { control: 'boolean' },
    options: { control: 'text' },

I wonder if the problem with this story is that options should be an array of strings or an array of objects with a label member, not just text. Or maybe text here just means they will take whatever you put in it and parse it into an object.

---

src/renderer/components/papi-components/Textfield.tsx line 49 at r7 (raw file):

Previously, rolfheij-sil wrote…

I'm not really sure what it needs, it's a bit obscure. MUI decided what these event handlers should look like based on the TextField variant you choose (with the variant prop). So I don't know what we can say about these without knowing if we're dealing with an outlined or a filled TextField

Do we particularly need to worry about variants right now? Is it just a stylistic difference, or is there functional difference as well? Why are there two different events fired based on the variant? Maybe we can look more closely at this together soon.

[katherinejensen00]

Reviewable status: 34 of 46 files reviewed, 14 unresolved discussions (waiting on @irahopkinson, @katherinejensen00, @rolfheij-sil, and @tjcouch-sil)

---

package.json line 67 at r4 (raw file):

Previously, rolfheij-sil wrote…

To be honest I don't know what it does, I guess it's part of the default/suggested configuration. @katherinejensen00 Do you know what this does?

That is the default alias for storybook build. I renamed the alias as TJ suggested to avoid confusion so it doesn't look like it is part of the overall build process. The new command is npm run package:storybook

---

package.json line 67 at r4 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

I'm wondering if you get this to build you will solve the ComboBox error, or when you solve the ComboBox error this might build correctly. However if no one knows what it does or even uses it then take it out until its needed.

@irahopkinson Are you still getting this error after the latest changes? I don't think I have gotten this one before and I am having a hard time reproducing it. Maybe we can pair on it if you are still getting it.

[irahopkinson]

Nearly there

Reviewed 12 of 12 files at r10, all commit messages.
Reviewable status: all files reviewed, 17 unresolved discussions (waiting on @katherinejensen00, @rolfheij-sil, and @tjcouch-sil)

---

package.json line 67 at r4 (raw file):

Previously, katherinejensen00 wrote…

That is the default alias for storybook build. I renamed the alias as TJ suggested to avoid confusion so it doesn't look like it is part of the overall build process. The new command is npm run package:storybook

I wonder if you looked at the older comments before TJ's? This is definitely not part of our package process as the other 2 above that start with that script name package. Please change this back to how Rolf fixed it as storybook:build. Remember to change the README also.

---

package.json line 67 at r4 (raw file):

Previously, katherinejensen00 wrote…

@irahopkinson Are you still getting this error after the latest changes? I don't think I have gotten this one before and I am having a hard time reproducing it. Maybe we can pair on it if you are still getting it.

Actually I can't even do an npm i at this commit (however I can at the merge commit before this one) so that needs to be fixed first However I really can't see what would affect that in this latest commit, weird.

I did go back one commit, do the install then back to this latest commit and I still ge the same error.

---

README.md line 78 at r10 (raw file):

Storybook

Thanks for adding this to the README. Please move this section to somewhere below ##Publishing as this relates to the section above ## Packaging for Production and Storybook is unrelated to either of them so should not be in the middle of them.

---

README.md line 86 at r10 (raw file):

To run storybook locally:

I would put this above the build so the 2 build options are documented together and how to run locally is probably what a developer needs to know first.

---

src/renderer/components/papi-components/Button.tsx line 33 at r10 (raw file):

 */
function Button({
  isPrimary = false,

Now that I've had more time to think about it, doesn't MUI have more than primary and secondary? I.e. you can have primary, secondary and neither, as well as outlined. That means a boolean can't describe the available options here. Unless we only mean to give them those 2 options?

---

src/renderer/components/papi-components/TextField.tsx line 72 at r10 (raw file):

  onFocus,
  onBlur,
}: TextfieldProps) {

The type above has a capital F. Revert this change.

Code quote:

TextfieldProps

[irahopkinson]

package.json line 67 at r4 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

Actually I can't even do an npm i at this commit (however I can at the merge commit before this one) so that needs to be fixed first However I really can't see what would affect that in this latest commit, weird.

I did go back one commit, do the install then back to this latest commit and I still ge the same error.

I traced the npm i issue down to the change of this file in this commit. I think there is a file corruption at the begining of the file. We might need to re-write this commit because we don't want that corruption in our repo. Let me know if you need help with this.

[rolfheij-sil]

Reviewable status: all files reviewed, 17 unresolved discussions (waiting on @irahopkinson and @tjcouch-sil)

---

.eslintrc.js line 2 at r6 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

This would be better like this:

  extends: [
     // https://github.com/electron-react-boilerplate/eslint-config-erb/blob/main/index.js
    // airbnb rules are embedded in erb https://github.com/airbnb/javascript/tree/master/packages/eslint-config-airbnb
    'erb',
    // https://github.com/storybookjs/eslint-plugin-storybook/blob/main/lib/configs/recommended.ts
    'plugin:storybook/recommended'
  ],

Done.

---

package.json line 67 at r4 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

I wonder if you looked at the older comments before TJ's? This is definitely not part of our package process as the other 2 above that start with that script name package. Please change this back to how Rolf fixed it as storybook:build. Remember to change the README also.

Done.

---

package.json line 67 at r4 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

I traced the npm i issue down to the change of this file in this commit. I think there is a file corruption at the begining of the file. We might need to re-write this commit because we don't want that corruption in our repo. Let me know if you need help with this.

I think I was able to fix it, as per your instructions on Discord

---

README.md line 78 at r10 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

Thanks for adding this to the README. Please move this section to somewhere below ##Publishing as this relates to the section above ## Packaging for Production and Storybook is unrelated to either of them so should not be in the middle of them.

Done.

---

README.md line 86 at r10 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

I would put this above the build so the 2 build options are documented together and how to run locally is probably what a developer needs to know first.

Done.

---

src/renderer/components/papi-components/Button.tsx line 44 at r6 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

@tjcouch-sil you can get the formatting you want if you use a tripple-backtick begining and end (even inline), i.e. className={`papi-button ${mode} className`}.

Done

---

src/renderer/components/papi-components/Button.tsx line 34 at r9 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

FYI this happens when you have an object being destructured and then rename one of the members of that object type. I guess TypeScript thinks it's being helpful by not renaming the variable created by the destructure. So you just kinda have to keep an eye out for this kind of thing when renaming type members. An easy tweak to fix this is to rename disabled to isDisabled on this line as well. Then it will have isDisabled: isDisabled on this line (and will use isDisabled below), so you can shorten it to just isDisabled

Done. Well spotted. I did as TJ described for all props that I've changed, but I slipped up with this one.

---

src/renderer/components/papi-components/Button.tsx line 33 at r10 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

Now that I've had more time to think about it, doesn't MUI have more than primary and secondary? I.e. you can have primary, secondary and neither, as well as outlined. That means a boolean can't describe the available options here. Unless we only mean to give them those 2 options?

None of the MUI components that we use have a primary prop. I think it was added by us for the sake testing how to change a component's styling using a boolean prop. But we can use a different data type for it, or remove it altogether.

---

src/renderer/components/papi-components/ComboBox.tsx line 34 at r6 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

https://mui.com/material-ui/react-autocomplete/#options-structure
Looks like they expect an array of objects that have label: string if you don't override their getOptionLabel. As such, I would be in favor of making ComboBox generic as I mention in a thread below. We can make it something like type MyOptionType = string | { label: string } | Record<string, unknown> and ComboBox<T extends MyOptionType>. Then we can explain that they need to put a label on each option unless they override getOptionLabel. Or we can not give them the option to override getOptionLabel right now and just tell them they have to provide a label like type MyOptionType = string | { label: string } - that might actually be a cleaner way for now :)

Let's pair on this one later today

---

src/renderer/components/papi-components/ComboBox.tsx line 45 at r6 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

I believe Ira was suggesting we declare new types that are simple aliases over the existing types. Like at the top of this file you would want to write export type ComboBoxChangeDetails = MuiComboBoxChangeDetails;, then you can use ComboBoxChangeDetails here. That way, it is our own type, and we can do what we want with it.

Let's pair on this one too

---

src/renderer/components/papi-components/ComboBox.tsx line 58 at r6 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

We may want to consider making this function a generic with a type constraint to make sure it functions as an options object like function Combobox<T extends MyOptionsType>({...}) {...}, but I don't know if it's really arguably a good idea. I will put it in the code style discussion list. Feel free not to take action on this until we discuss it.

We'll look into this one together too

---

src/renderer/components/papi-components/Switch.tsx line 36 at r5 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

Did I miss a discussion? I'm surprised to see this removed since onClick is the most used thing for any component.

Sorry, I removed it because I thought it was redundant because onChange is basically the same thing. I saw that the Slider also only has onChange and onChangeComitted. I can restore it if you want

---

src/stories/ComboBox.stories.tsx line 13 at r9 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

I wonder if the problem with this story is that options should be an array of strings or an array of objects with a label member, not just text. Or maybe text here just means they will take whatever you put in it and parse it into an object.

Let's have a look at it while pairing on ComboBox.tsx soon

---

src/stories/Textfield.stories.ts line 1 at r7 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

Er... you still need to change the filename. Note my post above about how to do that in Windows rather than Git.

Yep, sorry. Done.

---

src/renderer/components/papi-components/TextField.tsx line 72 at r10 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

The type above has a capital F. Revert this change.

Done.

---

src/renderer/components/papi-components/Textfield.tsx line 49 at r7 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

Do we particularly need to worry about variants right now? Is it just a stylistic difference, or is there functional difference as well? Why are there two different events fired based on the variant? Maybe we can look more closely at this together soon.

Yep, let's pair on this one too

---

src/renderer/components/papi-components/Textfield.tsx line 60 at r7 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

This is not a Git problem but a Windows problem. So you don't need to add Git commits to fix it. I usually rename the file by adding a character, e.g. from Texfield.ts to Text-field.ts, then removing and changing case at the same time, i.e. to TextField.ts. If you go directly from Texfield.ts to TextField.ts Windows changes the name you see but not the underlying name on the filesystem so Git sees no change but we do see the change and assume Git has the issue. I'm also post this in Discord for others to note.

Thanks Ira, I figured it was a Windows issue or an issue with my specific Git client, but I needed to get the work in before I ended my work week, so I didn't research any proper solutions and went for the dumb but easy one. I'll remember to use the approach you suggested next time

[irahopkinson]

Reviewed 6 of 6 files at r11, all commit messages.
Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @rolfheij-sil and @tjcouch-sil)

---

.eslintrc.js line 2 at r6 (raw file):

Previously, rolfheij-sil wrote…

Done.

Err... I don't see that it's done. Perhaps you missed staging it for the commit.

---

src/renderer/components/papi-components/Button.tsx line 33 at r10 (raw file):

Previously, rolfheij-sil wrote…

None of the MUI components that we use have a primary prop. I think it was added by us for the sake testing how to change a component's styling using a boolean prop. But we can use a different data type for it, or remove it altogether.

I think what we want is just to remove it since it can be controlled with more options by including what we want in className.

---

src/renderer/components/papi-components/Switch.tsx line 36 at r5 (raw file):

Previously, rolfheij-sil wrote…

Sorry, I removed it because I thought it was redundant because onChange is basically the same thing. I saw that the Slider also only has onChange and onChangeComitted. I can restore it if you want

Yeah, I guess you are right for a switch (as opposed to a button). Lets leave it out for now since its dead easy to add back in should we find we need it, but leaving it out makes for less clutter of options.

[rolfheij-sil]

Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @irahopkinson and @tjcouch-sil)

---

.eslintrc.js line 2 at r6 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

Err... I don't see that it's done. Perhaps you missed staging it for the commit.

Sorry, added it now!

---

src/renderer/components/papi-components/Button.tsx line 33 at r10 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

I think what we want is just to remove it since it can be controlled with more options by including what we want in className.

Done.

[tjcouch-sil]

Looks great! We're so close!! Keep up the great work!

Reviewed 1 of 12 files at r10, 3 of 6 files at r11, 5 of 7 files at r12, 6 of 6 files at r13, 5 of 5 files at r14, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @irahopkinson and @rolfheij-sil)

---

README.md line 111 at r13 (raw file):

```bash
npm run storybook:build {path to storybook-static folder}

Might this need to be npx http-server ./storybook-static from https://github.com/paranext/paranext-core#formatting-and-linting ? And this would need to be run after npm run storybook:build, so it may be best to mention that you have to build it first.

[tjcouch-sil]

Reviewed 1 of 12 files at r10, 1 of 1 files at r15, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @irahopkinson)

[irahopkinson]

So close.

Reviewed 5 of 7 files at r12, 6 of 6 files at r13, 4 of 5 files at r14, 1 of 1 files at r15, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @rolfheij-sil)

---

src/stories/Button.stories.tsx line 17 at r15 (raw file):

type Story = StoryObj<typeof Button>;

export const Primary: Story = {

This now needs 'primary' in the 'className' (assuming you revert the CSS changes as suggested above). Same for Switch below.

---

src/renderer/components/papi-components/button.css line 10 at r4 (raw file):

  line-height: 1;
}
.papi-button.primary {

I'd revert these deletions. We still want to be able to specify primary or secondary or neither in the buttons className.

---

src/renderer/components/papi-components/switch.css line 5 at r4 (raw file):

}

.papi-switch.primary {

Again I would revert these changes. We still want the option to select them using the switch's className.

[rolfheij-sil]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @irahopkinson)

---

README.md line 111 at r13 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

Might this need to be npx http-server ./storybook-static from https://github.com/paranext/paranext-core#formatting-and-linting ? And this would need to be run after npm run storybook:build, so it may be best to mention that you have to build it first.

Done

---

src/stories/Button.stories.tsx line 17 at r15 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

This now needs 'primary' in the 'className' (assuming you revert the CSS changes as suggested above). Same for Switch below.

Done.

---

src/renderer/components/papi-components/button.css line 10 at r4 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

I'd revert these deletions. We still want to be able to specify primary or secondary or neither in the buttons className.

Done.

---

src/renderer/components/papi-components/switch.css line 5 at r4 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

Again I would revert these changes. We still want the option to select them using the switch's className.

Done.

[irahopkinson]

Alright, made it through, good job everyone!

FYI: I'm still getting the same error on npm run storybook:build but that doesn't need to hold up htis PR since it seems like its only me. @katherinejensen00 perhaps you could help me with that tomorrow.

Reviewed 6 of 6 files at r16, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @rolfheij-sil)

[rolfheij-sil]

Thanks! And thank you guys for all the feedback :)

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @rolfheij-sil)