Common issues caught in PR review

[jattasNI]

🧹 Tech Debt

We'll use this issue to collect common issues found in PR review. Put the issues in comments, feel free to edit other people's comments with more info (does everyone have that permission?)

Once we've collected enough we can figure out what to do with the list. Maybe a code review checklist, maybe training material, maybe automated lint rules to catch this stuff for us, maybe capturing it in CONTRIBUTING in more detail.

[jattasNI]

Beachball change files

1. wrong change type
2. description is vague or focuses on implementation rather than client impact
3. use the GitHub obfuscated email
4. bump of monorepo package that is a devDep should use "dependentChangeType": "none" to prevent unnecessary new release of packages using it

[jattasNI]

Testing

1. Page objects should not expose Element types as they leak implementation detail and expose too much API surface area. Instead they should expose methods that access specific properties of the element and return primitive types.
2. All functionality should be covered by auto tests
3. Tests should be at the appropriate level (unit test or Chromatic test)
4. If something's not covered by auto tests, the PR description should include justification

[jattasNI]

Storybook

1. Everything in the public API that we expect clients to use should be documented, including important events and methods. Documentation should follow the patterns of similar APIs on other components.
2. No new accessibility violations were introduced

[jattasNI]

Styles

1. Styles should be ordered according to our CSS conventions
2. Don't set size attributes to inherit See examples in discussion 1 and discussion 2
3. For sizing, use appropriate size tokens for the right context (i.e. control height), but for sizes without a context aligned token, use a size from the 4px grid size ramp.
   • Do not hard code specific pixel values, use the size ramp
   • If you need to add additional values to the size ramp, see the latest naming guidance decision
   • Yes the naming system for the fixed pixel size ramp is not great and uses the term "padding". It should still be used.
4. The unset and revert keywords probably don't do what you think they do. Read the mdn docs carefully; they are probably not part of the solution you are looking for.
5. Be aware of the display modes being applied to a elements in a template. Largely we should be using one of the layouts flex/grid (block) or inline-flex/inline-grid (inline). If there are a lot of different display modes being used to layout a template that may be a sign to double check the layout at a high-level.

[jattasNI]

Component index.ts

1. Don't write logic in TypeScript if it can be expressed in the template or style instead. e.g. no size or style calculations in TS. No DOM manipulation in TS.

[jattasNI]

PR structure

1. PRs should do one thing and should not be too large
2. PR title and description should be clear and detailed

[mollykreis]

TypeScript

1. Disabling lint rules should be rare and have good justification
2. Use of non-null assertions (i.e. !) should be specific to places where TypeScript could not infer the type correctly. It should not be used to avoid properly handling corner cases in the code.

[rajsite]

Buddy Reviews

The Nimble Code Review process states that contributions from outside the Nimble dev team should have reviews from a member of the Nimble Team before moving to Owners review.

If you are outside the NI organization, create an issue for discussion ideally before creating a PR.

If you are inside the NI organization and new to contributing to Nimble reach out on the Design System Teams channel to discuss your goals and get a Nimble point of contact.

Current Nimble Contacts:

• Wafer Map Component @atmgrifter00
• NIC 2 @mollykreis
• LIMS Rich Text / Comments @m-akinc

[rajsite]

Component Templates

1. Don't use nullish coalescing in bindings in templates (other kinds of conditional logic should behave well). However, don't use any conditional logic in aggregate bindings (i.e. like when class is stitched together with multiple bindings). See detailed examples: https://github.com/ni/nimble/issues/1843#issuecomment-2064663561

[rajsite]

Related Changes

1. We strive for consistency across components. When making a change in one spot, make sure to consider how that relates to out other components.
2. We have three listbox / dropdown implementations (as of writing), select, combobox, and user mention listbox. A change in one should be considered against all three.

[rajsite]

Upgrading dependencies

In the course of updating dependencies you may find that some dependencies get upgraded with a semver range that actually breaks the build. For example:

• We have a dependency on "awesome-dep": "^1.0.0"
• Our package-lock.json has a resolved dependency on awesome-dep@1.0.1 which is great, a patch release that does not break an API and fixes a bug.
• An unrelated package is updated, etc, that incidentally updates the package-lock.json so it has a resolved dependency on awesome-dep@1.0.2 which is still in our declared semver range and would hopefully be great and fix a bug but, ah!, the package maintainers accidentally released a version that broke our build 😢
• Do not try and tweak the package-lock.json by doing different incantations to avoid that package updating and get a passing build. You have found that our supported semver range is actually broken. The repo is in a bad state. We don't actually support awesome-dep@^1.0.0 because some of those patch or minor releases ended up being breaking.
  • What does it mean for the repo to be in a bad state? What is happening is that what we declare as a supported semver range, "awesome-dep": "^1.0.0", is not actually supported. It is particularly bad if it is a dependency instead of a devDependency. It means that potentially a new app that is created or a down stream client that rebuilds their lock file etc, will hit an issue leveraging our package because our own declared "supported" dependency range breaks us. It's unearthing a ticking time bomb that will blow. Something to address ASAP.
• Do feel a little sad that you are temporarily blocked and try to resolve the issue. It may be package-specific but some options:
  • Rebuild the lock file from scratch to make sure everything is up-to-date by locally deleting the package-lock.json, doing a git clean -fdx to purge all node_modules, and running npm install to create a new lock file.
  • If the new clean lock file is still failing the build you may need package.json updates to resolve to supported versions. Some options:
  • fix the underlying issue and update the semver supported range to the new minimum that is supported, i.e. "awesome-dep": "^1.0.2", rebuild the lock file, and submit
  • pin a dependency, i.e. "awesome-dep": "1.0.1" (the last good known version), rebuild the lock file and see if the new resolved version fixes the issue, and create a tech debt item to unpin the version (and make sure it moves towards being fixed!)

[rajsite]

Handling unstable stories

If you see a chromatic diff for unstable stories (such as the menu stories) verify the snapshots but do not accept bad snapshots. Instead to create good snapshots you should be able to do the following:

• Bump the GUID in preview.js to rebuild all snapshots.
• Verify that all snapshots are being taken (i.e. zero turbo snaps)
• Use the rebuild command to see if the stories settle:
• Note: You may need 1-2 cycles of modifying the token in preview.js and using the rebuild command

[rajsite]

Avoid memory leaks

• Make sure event registrations, i.e. addEventListener and removeEventListener are cleaned up in the disconnectedCallback
• Make sure notifier subscriptions, i.e. Observable.getNotifier and notifierRef.subscribe are cleaned up in the disconnectedCallback