Better commenting and unify code formatting #49
Conversation
david-macpherson
changed the title
There was a problem hiding this comment.
This is an improvement but I think we can still further improve the code commenting/documenting. I've left some feedback and I think another pass over the code is necessary, with a focus on verbosity.
Perhaps we should also consider adding some legal boilerplate to each file. Probably worth a discussion with @sarahkrivan
| @@ -4,7 +4,7 @@ export class LoadingOverlay extends TextOverlay { | |||
|
|
|||
| private static _rootElement: HTMLElement; | |||
| private static _textElement: HTMLElement; | |||
| private static _spinner: HTMLElement; | |||
| private static _spinnerElement: HTMLElement; | |||
|
|
|||
| /** | |||
| * @returns The created root element of this overlay. | |||
There was a problem hiding this comment.
We could further expand commenting here to explain 'what is the root element' and why it's needed, what its purpose is etc. (just to make it obvious)
There was a problem hiding this comment.
I couldn't add a comment on the exact lines, but
export class LoadingOverlay extends TextOverlay {Above the class definition, I think we should include some commenting explaining what the class is, it's basic function, core functionality, etc
| @@ -31,27 +31,27 @@ export class LoadingOverlay extends TextOverlay { | |||
|
|
|||
|
|
|||
| public static spinner(): HTMLElement { | |||
There was a problem hiding this comment.
Could use some commenting explaining what this function does
| const SpinnerSectionOne = document.createElement("div"); | ||
| SpinnerSectionOne.setAttribute("style", "width: " + size * 0.8 + "px; height: " + size * 0.8 + "px; border-width: " + size * 0.125 + "px;"); | ||
| LoadingOverlay._spinner.appendChild(SpinnerSectionOne); | ||
| LoadingOverlay._spinnerElement.appendChild(SpinnerSectionOne); | ||
|
|
||
| const SpinnerSectionTwo = document.createElement("div"); | ||
| SpinnerSectionTwo.setAttribute("style", "width: " + size * 0.8 + "px; height: " + size * 0.8 + "px; border-width: " + size * 0.125 + "px;"); | ||
| LoadingOverlay._spinner.appendChild(SpinnerSectionTwo); | ||
| LoadingOverlay._spinnerElement.appendChild(SpinnerSectionTwo); | ||
|
|
||
| const SpinnerSectionThree = document.createElement("div"); | ||
| SpinnerSectionThree.setAttribute("style", "width: " + size * 0.8 + "px; height: " + size * 0.8 + "px; border-width: " + size * 0.125 + "px;"); | ||
| LoadingOverlay._spinner.appendChild(SpinnerSectionThree); | ||
| LoadingOverlay._spinnerElement.appendChild(SpinnerSectionThree); |
There was a problem hiding this comment.
What is SpinnerSectionOne, SpinnerSectionTwo and SpinnerSectionThree? Can we add commenting explaining the purpose of this being done the way it is and what these separate div elements relate to
| } | ||
| return LoadingOverlay._spinner; | ||
| return LoadingOverlay._spinnerElement; | ||
| } | ||
| /** | ||
| * Construct a connect overlay with a connection button. |
There was a problem hiding this comment.
This says construct a connect overlay yet we call the class LoadingOverlay. This doesn't seem relevant
| export class SPSApplication extends Application { | ||
| private loadingOverlay: LoadingOverlay; | ||
| private signallingExtension: SPSSignalling; | ||
|
|
||
| // Create a flags class for the send stats to server |
There was a problem hiding this comment.
I think this can use some more explanation (perhaps why we do this and even a link to how PSInfra docs on how we know how to add custom stats).
|
|
||
| // For local testing. Declare a websocket URL that can be imported via a .env file that will override | ||
| // the signalling server URL builder. | ||
| declare var WEBSOCKET_URL: string; | ||
|
|
||
|
|
||
| export class SPSApplication extends Application { |
There was a problem hiding this comment.
What is SPSApplication and what purpose does it serve in this example implementation?
Relevant components:
Problem statement:
A tidy up of commenting was required for the library and the examples
Solution
Add better commenting to the SPS library and examples