feat: converted code to typescript #25
Conversation
There was a problem hiding this comment.
Overall looks ok, what I feel it is needed is better jsdoc docs for all functions in the project. For open-source projects this is essential to read and understand code.
There's a vscode extension called "Document This" that helps with most of the properties necessary with jsdoc. But functions need short and concise descriptions.
|
@JoaoTMDias Documentation added! |
DiogoRDuarte
force-pushed
the
feat-conversion-typescript
branch
from
c80f431
to
ece342d
Compare
src/AutoVizuA11y.tsx
Outdated
| }; | ||
|
|
||
| /** | ||
| * AutoVizuA11y component, that adds screen-reader accessibility to wrapped charts. |
There was a problem hiding this comment.
Please remove the comma.
src/AutoVizuA11y.tsx
Outdated
| * @param {AutoVizuA11yProps} { | ||
| * type, - type of chart; | ||
| * descriptor, - descriptor of each data element; | ||
| * selectorType, - HTML type or classname of the data elements in the DOM; |
There was a problem hiding this comment.
Extra space between "or" and "classname"
src/ShortcutGuide.tsx
Outdated
| @@ -8,7 +8,12 @@ | |||
| import React, { useRef, useEffect } from "react"; | |||
| import "./style/ShortcutGuideStyle.css"; | |||
|
|
|||
| const ShortcutGuide = () => { | |||
| /** | |||
| * Component that renders the list (also visual) of all AutoVizuA11y shortcuts. | |||
There was a problem hiding this comment.
Does the "also visual" matter?
There was a problem hiding this comment.
I agree it can be cut
|
|
||
| //When pressed reads the smaller description | ||
| /** | ||
| * Handles the longer and shorter description change when Alt+B or Alt+S are pressed, respectively. |
There was a problem hiding this comment.
The alt+B and alt+S command is not for MacOS. Can you add the "option" detail here and everywhere you mention shortcuts in the documentation?
| import { addDataNavigation } from "./AddNavigation"; | ||
|
|
||
| /** | ||
| * Changes navigation based on key press. |
There was a problem hiding this comment.
What do we mean by "changes navigation" here?
| } | ||
|
|
||
| /** | ||
| * Enables navigation on another data series. |
There was a problem hiding this comment.
It's more something like "Enables navigation between data series", no?
| export function overallComparer(event, alertDiv, insights, arrayConverted, focusedData) { | ||
| const { nativeEvent } = event; | ||
| /** | ||
| * Produces a message based on corner cases and calls comparer otherwise. |
There was a problem hiding this comment.
What exactly do you mean about the "message based on corner cases"? Is that the focus of the function or is this secondary?
src/components/shortcuts/Skip.ts
Outdated
| */ | ||
|
|
||
| /** | ||
| * Enables one to jump to the end/beginning of a chart. |
There was a problem hiding this comment.
Just for the phrasing to be more aligned with the others, I suggest writing something like "Enables the navigation to the end/beginning of a chart"
src/utils/maths.ts
Outdated
| //calculates the median value | ||
| export function median(arr) { | ||
| /** | ||
| * Calculates the median value of a number array. |
There was a problem hiding this comment.
a number array -> an array of numbers
src/utils/maths.ts
Outdated
| //returns the ordinal number based on a number | ||
| export function getOrdinalNumber(number) { | ||
| /** | ||
| * Determines the ordinal version of a number. |
There was a problem hiding this comment.
Maybe something like "Transforms number to ordinal". Wdyt?
feat: converted code to typescript
Because
This Commit
Refactor