Differences

This shows you the differences between two versions of the page.

--- dev:angular_dev_best_practices [2019/01/18 11:32] – erickson
+++ dev:angular_dev_best_practices [2024/09/15 13:03] (current) – [Running ng lint] add info about prerequisites sandbergja
@@ Line 4: / Line 4: @@
 Loose collection of practices to review with developers so we can agree on UI developer guidelines.
+==== Code quality ====
-  * Run 'ng build --prod' before committing.
+  * Run %%'ng build --prod'%% before committing.
     * This ensures the templates also compile successfully.
-  * Run 'ng lint' before committing.
+  * Run 'ng lint' before committing, see details below.
-    * This does more than check style, it also warns when an import pulls in too much code, among other things.
+  * Make sure that 'ng extract-i18n' runs before committing (some i18n syntax errors are not caught by ng build)
   * use camelCase variables when possible for consistency.  (Note fields on IDL objects don't apply).
+  * Use 2-space indentation in HTML files
+  * Use 4-space indentation in Typescript and CSS files.
+  * Avoid column widths in code that extend well beyond 80-100 characters
+==== UI ====
   * Design for screen width of 1350 pixels.
     * This is somewhat arbitrary, but seems to cover most laptops and smaller (non-mobile) screens without being too limiting.
     * Browser dev tools can show the width of the current browser window.
-  * Use 2-space indentation in HTML files
-  * Use 4-space indentation in Typescript and CSS files.
-  * Avoid column widths that extend well beyond 80-100 characters
   * Use Bootstrap utility classes when possible instead of writing custom CSS.
     * https://getbootstrap.com/docs/4.2/utilities/
@@ Line 23: / Line 27: @@
   * Give form inputs ID values with linked i18n labels
     * <label for="patron-name" i18n>Name</label><input id="patron-name" .../>
     * Beware repeatable components have unique form input IDs (extend id with incrementor, random(), etc.)
+  * Give buttons meaningful title or aria-label attributes
+  * Give images meaningful alt attributes (alt="" is okay for decorative or redundant images; see the [[https://www.w3.org/WAI/tutorials/images/decision-tree/|W3C decision tree for alt text]])
+  * Test keyboard navigation with tab, shift+tab, arrow keys, and other keys as appropriate. Make sure that all buttons, links, and other interactive elements are focusable.
+    * The [[https://www.w3.org/TR/wai-aria-practices-1.1/|WAI-Aria Authoring Practices]] may help you determine the appropriate keyboard interactions for more complex widgets
+    * Don't add Bootstrap's "btn" class to unfocusable elements, like <span>s, <label>s, or <a>s that don't also have a "href" attribute.  Use a tag that is already focusable instead, like <button> or <a> with a "href" attribute.
+  * Make sure that your screens have an <eg-staff-banner> at the top to let users know where they are.
+==== ng lint ====
+We use a tool called ''ng lint'' in the Angular client to keep our code readable, consistently formatted, and to avoid some common bugs.  Under the hood, ''ng lint'' uses a tool called eslint -- the [[https://eslint.org/|eslint website]] is a good place to find documentation about the lint rules.
+=== Running ng lint ===
+You should run ng lint early and often in the development process, as it can help you avoid certain bugs.  Definitely run it before submitting your code for community review, or before committing any Angular code to the Evergreen repository.
+== Preparing to run ng lint ==
+Ng lint does not require a running Evergreen system.  You just need to install a few dependencies before running it for the first time.
+  - [[newdevs:git:install|Install git and clone the Evergreen git repository]]
+  - [[https://nodejs.org/en/download/package-manager|Install NodeJS using one of the options on their download page]].
+  - In a new terminal, cmd, or powershell window, use the cd command to navigate to the Open-ILS/src/eg2 directory within the Evergreen git repository
+  - Run the command ''npm install''
+  - Optionally, install the Angular cli globally with ''npm install -g @angular/cli@^15'' (replace 15 with the version of Angular that Evergreen currently uses, which you can find in the file Open-ILS/src/extras/install/Makefile.common).
+== Running the ng lint command ==
+  * If you have the Angular cli installed, you can run the following command from the eg2 directory: ''ng lint''
+  * If you don't have the Angular cli installed, you can run the following from the eg2 directory: ''npx ng lint''
+  * If you use VS Code, Microsoft makes a nice eslint extension that can catch problems as you type.
+  * Finally, [[https://github.com/evergreen-library-system/Evergreen|Evergreen's Github]] automatically runs ''ng lint'' any time it sees a commit to the eg2 directory.
+=== Addressing issues found by ng lint ===
+  * ''ng lint'' can automatically fix many issues for you.  To do this, run ''ng lint --fix''
+  * You can tell ''ng lint'' to ignore a certain line if needed.  For example, if you want to ignore the error "rxjs/no-implicit-any-catch", you could add a comment before that line:
+<code javascript>
+// eslint-disable-next-line no-var
+var myVariable = 1;
+</code>
+It's a good practice to also say a bit more in this line about why we can't do the version lint would prefer, for example:
+<code javascript>
+// eslint-disable-next-line eqeqeq -- we have to use ==, rather than ===, here since result could be a string or an integer
+if (result == '2') {
+</code>
+== Common lint errors ==
+  * ''no-magic-numbers'': A magic number is a number that you use in your code without explaining what it represents.  To resolve this error, rather than using your number directly (e.g. ''86400''), first assign it to a variable with a descriptive name (e.g. ''const oneDayInSeconds = 86400''), and use the variable instead.
+  * ''no-shadow'': You have two variables with the same name in two different scopes (for example, you have a variable named ''data'' in your method, and then you go into a loop that uses a different variable also named ''data'').  You can resolve this by renaming one (or both) of the variables to something more descriptive.
+  * ''no-cond-assign'': Ng lint will complain if you do something like ''if (myValue = 3)'', because it's very likely that you meant to compare myValue and 3 (''if (myValue === 3)''), rather than set the value of myValue to 3. If that's what you meant, you can simply add the extra equal signs.  If you do indeed want to assign ''myValue'', consider if it might be possible or clearer to assign it before the conditional, or if you can [[https://eslint.org/docs/latest/rules/no-cond-assign#except-parens|use parentheses to make it clearer that you are both assigning and comparing in one step]].
+  * ''rxjs/no-nested-subscribe'': The angular client uses RxJS Observables frequently, especially when fetching data from the server.  You may sometimes wish to fetch some data from the server, then, depending on what is in the data, fetch some more.  For example, if you want to find some items, set them to deleted, then log their call number to the console, it can be tempting to write something like this:
+<code>
+// This code contains 2 rxjs/no-nested-subscribe errors
+const idsToDelete = [3, 12, 30];
+this.pcrud.search('acp', {id: idsToDelete}).subscribe(item => {
+  item.deleted(true);
+  this.pcrud.autoApply(item).subscribe(deletedItem => {
+    this.pcrud.search('acn', {id: deletedItem.call_number()}).subscribe(callNumber => {
+	  console.log(`We deleted an item related to call number ${callNumber.label()}`);
+	});
+  });
+});
+</code>
+Note that there are three `subscribe()` calls, nested inside of each other, each of which will probably handle three events (for each of the ids we want to change).  Troubleshooting this complexity can get tricky!  Instead, you could refactor the above code to use RxJS switchmap, which is built for accepting data from one observable, then switching to a different observable that uses the same data.
+<code>
+// This code has no rxjs/no-nested-subscribe errors
+const idsToDelete = [3, 12, 30];
+this.pcrud.search('acp', {id: idsToDelete}).pipe(
+  switchMap(item => {
+    item.deleted(true);
+    return this.pcrud.autoApply(item);
+  }),
+  switchMap(deletedItem => {
+    return this.pcrud.search('acn', {id: deletedItem.call_number()});
+  })
+).subscribe(callNumber => {
+  console.log(`We deleted an item related to call number ${callNumber.label()}`);
+});
+</code>
+In this code, the distinction between the different calls and their sequence of events is a bit clearer at a glance, making for easier troubleshooting.
+==== Unit Tests ====
+  * Files named *.spec.ts are automatically treated as unit test files.
+  * Unit test files should live in the same directory as the code they are testing.
+    * e.g. idl.service.ts and idl.spec.ts are in the same directory.
+  * See Open-ILS/src/eg2/src/app/core/*.spec.ts files unit test examples of core services.
+  * The highest priority for unit test implementation are shared *.service.ts files, followed by shared components, since changes to these files have the broadest impact.
+{{tag>automated-tests}}