Skip to content

test code for review #3

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

test code for review #3

wants to merge 5 commits into from

Conversation

Smita81
Copy link
Collaborator

@Smita81 Smita81 commented May 6, 2024

No description provided.

@Smita81 Smita81 self-assigned this May 6, 2024
@Smita81 Smita81 requested review from dannyelcf and pallavisarwar May 6, 2024 10:34
@@ -0,0 +1,14 @@
export const INITIAL_STATE = {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Run npm run format to correctly indent the code.

@@ -0,0 +1,20 @@
import { INITIAL_STATE, EVENTS, HTML_CLASSES } from './constants.js';
const handlers = (EVENTS) => {
Copy link
Owner

@dannyelcf dannyelcf May 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The name of the arguments must not be UPPERCASED.

Suggested change
const handlers = (EVENTS) => {
const handlers = (event) => {

const eventsMouseOverHistory = document.getElementById(EVENTS.MOUSE_OVER_HISTORY,);
// Handler logic for when the user moves the mouse over 'number-history'
*/
const targetText = EVENTS.target.innerText;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const targetText = EVENTS.target.innerText;
const targetText = event.target.innerText;


backwardsContainer.innerHTML = liString + backwardsContainer.innerHTML;

if (EVENTS.target.nodeName !== 'LI') {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (EVENTS.target.nodeName !== 'LI') {
if (event.target.nodeName !== 'LI') {

return;
};

const liString = `<li class="number-item">${state.currentNumber}</li>`;s
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove the s at the end of the line.

Suggested change
const liString = `<li class="number-item">${state.currentNumber}</li>`;s
const liString = `<li class="number-item">${state.currentNumber}</li>`;

@@ -1,37 +1,37 @@
debugger; // once when the program is initialized
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once you have implemented the file listeners.js you must remove all the code inside init.js and import the file listeners.js here.

import './listeners.js'

Comment on lines 2 to 5
const state = {
currentNumber: 0,
allNumbers: [],
};
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This snippet of code must be inside a state.js file.

Comment on lines 10 to 14
const nextNumber = Number(inputValue);

// --- update state ---
state.allNumbers.push(nextNumber);
state.currentNumber = state.allNumbers.at(-1);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This snippet of code must be inside a handler code.

@@ -0,0 +1,14 @@
import { INITIAL_STATE, EVENTS, HTML_CLASSES } from './constants.js';
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An utils file must contain exported functions. Here in your file you didn't create any function.

Comment on lines 10 to 19
for (const array of arrayOfArrays) {
const tr = document.createElement('tr');
table.appendChild(tr);
for (const element of array) {
const td = document.createElement('td');
td.innerText = element;
tr.appendChild(td);
}
table.appendChild(tr);
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When we have an arrayOfArrays represeting a table, usually we use names like row and column in the for loops.

Suggested change
for (const array of arrayOfArrays) {
const tr = document.createElement('tr');
table.appendChild(tr);
for (const element of array) {
const td = document.createElement('td');
td.innerText = element;
tr.appendChild(td);
}
table.appendChild(tr);
}
for (const row of arrayOfArrays) {
const tr = document.createElement('tr');
table.appendChild(tr);
for (const column of row) {
const td = document.createElement('td');
td.innerText = column;
tr.appendChild(td);
}
}

td.innerText = element;
tr.appendChild(td);
}
table.appendChild(tr);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this line. You already did this in the line 12.

const summaryEl = document.createElement('_');
_;
_;
const summaryEl = document.createElement('summery');
Copy link
Owner

@dannyelcf dannyelcf May 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You made a typo here.

Suggested change
const summaryEl = document.createElement('summery');
const summaryEl = document.createElement('summary');

} from '../constants.js';
export const convertedTemperature = (fahrenheitTextList) => {
if (!/^[0-9\s]*$/.test(fahrenheitTextList)) {
window.alert(MESSAGE_ERROR_NOT_INTEGER);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You defined MESSAGE_ERROR_NOT_INTEGER constant as The '%S' contains values different of integer numbers.

So, in the window.alert you must replace the %S by fahrenheitTextList content. For example:

window.alert(MESSAGE_ERROR_NOT_INTEGER.replace('%S', fahrenheitTextList));

Comment on lines +13 to +19
var convertedTemperatures = '';
fahrenheitList.forEach((fahrenheit) => {
// Do the math
const celsius = ((fahrenheit - 32) * 5) / 9;
convertedTemperatures = convertedTemperatures + ' ' + celsius.toFixed(2);
});
return convertedTemperatures;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't use var. It is not recommeded anymore. Use let or const.

Tip: use map and join functions. This will avoid the empty space at the beginning of the returned convertedTemperatures.

Suggested change
var convertedTemperatures = '';
fahrenheitList.forEach((fahrenheit) => {
// Do the math
const celsius = ((fahrenheit - 32) * 5) / 9;
convertedTemperatures = convertedTemperatures + ' ' + celsius.toFixed(2);
});
return convertedTemperatures;
const convertedTemperatures = fahrenheitList.map((fahrenheit) => {
const celsius = ((fahrenheit - 32) * 5) / 9;
return celsius.toFixed(2);
}).join(' ');

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants