add all files #1

danielmargolin · 2024-03-13T20:46:32Z

No description provided.

danielmargolin · 2024-03-13T20:50:53Z

rotmeNissim/Assignment_1_DevClub-main/script.js

@@ -0,0 +1,175 @@
+//formatting bytes to MB/KB/GB for UI.
+function formatSizeUnits(fileSize) {


Your function changes the given param
for example:
function name (x) { x = 1 }
bad practice

Here is a code example from chatgpt that achieves the same result:

function formatBytes(bytes) { const sizes = ['Bytes', 'KB', 'MB', 'GB', 'TB']; if (bytes === 0) return '0 Byte'; const i = parseInt(Math.floor(Math.log(bytes) / Math.log(1024))); return Math.round(bytes / Math.pow(1024, i), 2) + ' ' + sizes[i]; } // Example usage: console.log(formatBytes(1024)); // Output: 1 KB console.log(formatBytes(1048576)); // Output: 1 MB console.log(formatBytes(1099511627776)); // Output: 1 TB

danielmargolin · 2024-03-13T20:52:14Z

rotmeNissim/Assignment_1_DevClub-main/script.js

+
+   }
+//shrinking the file name for UI.   
+function shrinkFileName(fileName) {


Awesome idea, but can be achieved using just css
(read about ellipsis with css)

danielmargolin · 2024-03-13T20:56:26Z

rotmeNissim/Assignment_1_DevClub-main/script.js

+   }
+function updateLocalStorage() {
+
+    const storedFiles = JSON.parse(window.localStorage.getItem("selectedFiles")) || []; //holding the selected file as JSON object


You don't have to specify window before reaching localStorage.
You can just use localStorage

danielmargolin · 2024-03-13T20:56:41Z

rotmeNissim/Assignment_1_DevClub-main/script.js

+function updateLocalStorage() {
+
+    const storedFiles = JSON.parse(window.localStorage.getItem("selectedFiles")) || []; //holding the selected file as JSON object
+    console.log(storedFiles); //self-checking


redundant console.log
please remove

danielmargolin · 2024-03-13T20:57:10Z

rotmeNissim/Assignment_1_DevClub-main/script.js

+    const storedFiles = JSON.parse(window.localStorage.getItem("selectedFiles")) || []; //holding the selected file as JSON object
+    console.log(storedFiles); //self-checking
+    // As I was struggling with the CSS I tried to dynamically resize my container (lines 28-32)
+    const fileCount = storedFiles.length;


filesCount?

danielmargolin · 2024-03-13T21:06:06Z

rotmeNissim/Assignment_1_DevClub-main/script.js

+    window.localStorage.setItem("selectedFiles", JSON.stringify(updatedFiles)); //updating the local storage with the new set of items
+}
+//function that sums the size in bytes for each selected file
+function calcTotalSize(files) {


rename to getTotalSize (the name should suggest that there is a returned value)

danielmargolin · 2024-03-13T21:08:32Z

rotmeNissim/Assignment_1_DevClub-main/script.js

+    const text = document.getElementById("mb-left-num");
+    const usage = document.getElementById("storage-used");
+
+    const maxStorage = 100 * 1024 * 1024; //setting the max storage to 100MB.


Awesome that you are using bytes.
But this kind of property should be stored in a more global place.
Because if in the future, you will need to change it to 200MB, you won't have to search for this line

danielmargolin · 2024-03-13T21:09:09Z

rotmeNissim/Assignment_1_DevClub-main/script.js

+    alert("Not enough memory, please remove file/s and try again");
+    }
+    //validations
+    let percentage;


const percentage = totalSize > 0 ? (totalSize / maxStorage) * 100 : 0

danielmargolin · 2024-03-13T21:10:14Z

rotmeNissim/Assignment_1_DevClub-main/script.js

+    console.log("Percentage:", percentage);
+    progressBar.style.width = percentage + "%"; //changing the width of the progress bar according to variant.
+    //validations
+    let formattedSize;


const formattedSize = totalSize > 0 ? formatSizeUnits(totalSize) : "100 MB"

danielmargolin · 2024-03-13T21:13:04Z

rotmeNissim/Assignment_1_DevClub-main/script.js

+  if (files.length > 0) {
+    const selectedFiles = JSON.parse(window.localStorage.getItem("selectedFiles")) || []; //wether there are already files or not
+
+    const newFiles = Array.from(files).map(file => ({


Syntactic sugar:

const newFiles = Array.from(files).map(({name, size}) => ({ name, size }));

alonman17

@ Tal levi
Good job in general, please take a look at the comments

alonman17 · 2024-03-13T21:18:48Z

talLevi/fylo-solution-main/script.js

+}
+
+function updateStorageOnUpload (fileSize) {
+    var totalStorage = parseInt(localStorage.getItem('totalStorage')); //this makes char to int


Use let
let totalStorage = parseInt(localStorage.getItem('totalStorage'))
https://www.geeksforgeeks.org/difference-between-var-and-let-in-javascript/

alonman17 · 2024-03-13T21:20:06Z

talLevi/fylo-solution-main/script.js

+    document.getElementById('usedStorage').textContent = usedStorage;
+
+    var remainingStorage = totalStorage - usedStorage;
+    var percentageUsed = (usedStorage) ;


redundant variable

alonman17 · 2024-03-13T21:21:53Z

talLevi/fylo-solution-main/script.js

+document.getElementById('upload').addEventListener('click', uploadFile);
+
+function initializeStorage() {
+    // Check if the storage is already initialized


Reduce the amount of comments.
Clear and readable code should speak for itself

alonman17 · 2024-03-13T21:26:16Z

talLevi/fylo-solution-main/script.js

+    var usedStorage = parseInt(localStorage.getItem('usedStorage'));
+
+    totalStorage -= fileSize;
+    usedStorage += Math.ceil(fileSize);


usedStorage += (fileSize).toFixed(2);
Might be more appropriate

alonman17 · 2024-03-13T21:28:00Z

talLevi/fylo-solution-main/script.js

+    for (var i=0; i<selectedFiles.length; i++) {
+        var currentFile = selectedFiles[i];
+        var allowedFormats = ["image/jpeg", "image/jpg", "image/png", "image.gif"]; //this array includes the acceptable formats
+        var fileSize = (currentFile.size) / (1024) / (1024);


redundant ()

alonman17 · 2024-03-13T21:28:52Z

talLevi/fylo-solution-main/script.js

+
+function CheckSpace (fileSize) {
+    var totalStorage = parseInt(localStorage.getItem('totalStorage'));
+if (fileSize>totalStorage)


Indentations and spaces

alonman17 · 2024-03-13T21:30:39Z

talLevi/fylo-solution-main/script.js

+            fileInput.value = '';
+            return; 
+        }
+        CheckSpace(fileSize);


The function checkFileFormat should only have one responsibility.
CheckSpace should not be called here

alonman17 · 2024-03-13T21:34:28Z

talLevi/fylo-solution-main/script.js

+}
+
+function updateStorageOnUpload (fileSize) {
+    var totalStorage = parseInt(localStorage.getItem('totalStorage')); //this makes char to int


In general, the usage of totalStorage in local storage can be removed.
let totalStorage = 100 in the start of the file would have the same effect, with less calls to the localStorage

alonman17 · 2024-03-13T21:41:51Z

talLevi/fylo-solution-main/script.js

+function uploadFile() {
+
+    checkFileFormat();
+


Here would be the appropriate place to call CheckSpace() :)

into fyloCR

idanasayag0 · 2024-03-14T11:50:24Z

eyarGdolov/script.js

@@ -0,0 +1,86 @@
+var files=[];


use let instead var
let files = [];
https://www.geeksforgeeks.org/difference-between-var-and-let-in-javascript/

idanasayag0 · 2024-03-14T11:51:46Z

eyarGdolov/script.js

@@ -0,0 +1,86 @@
+var files=[];
+let maxSize=100;


the variable never change, use const.
const maxSize = 100;
https://www.w3schools.com/js/js_const.asp

idanasayag0 · 2024-03-14T11:53:06Z

eyarGdolov/script.js

+var files=[];
+let maxSize=100;
+
+<!-- This function displays a list of the uploaded images and can delete them from the list-->


There is no need to add comments before the functions, clean code is comprehensible code without comments.

idanasayag0 · 2024-03-14T12:05:08Z

eyarGdolov/script.js

+
+        if (!allowedExt.some(ext => file.name.toLowerCase().endsWith(ext))) {
+            alert("Error: File format is not supported.");
+            console.error('Error: File format is not supported.');


You should remove all the console.log before you commit.

idanasayag0 · 2024-03-14T12:12:14Z

eyarGdolov/script.js

+        if (!allowedExt.some(ext => file.name.toLowerCase().endsWith(ext))) {
+            alert("Error: File format is not supported.");
+            console.error('Error: File format is not supported.');
+        }


Shouldn't you stop the function when user try to upload unsupported file?

if (!allowedExt.some(ext => file.name.toLowerCase().endsWith(ext))) { alert("Error: File format is not supported."); return; }

In your case the user can still upload every file.

idanasayag0 · 2024-03-14T12:32:45Z

eyarGdolov/script.js

+            StorageLeft.innerHTML = MbLeft;
+            return MbLeft;
+        }
+        else {


This code is unreachable.
You already check the image size (in handleUserImages) before entering calcFileSize.
Meaning you can remove the if/else condition.

idanasayag0 · 2024-03-14T12:38:57Z

eyarGdolov/script.js

+    }
+}
+
+    <!-- This function opens the file explorer -->


You should create the input in the html instead here.
more clean.

in the html: <input type="file" onchange="handleUserImages()">

idanasayag0 · 2024-03-14T12:41:23Z

eyarGdolov/script.js

+            const MbLeft = maxSize - totalUsedSpaceMb;
+            console.log('MbLeft:', MbLeft);
+            const StorageUsed = document.getElementById('StorageUsed');
+            const StorageUsed1 = document.getElementById('StorageUsed1');


You should give variable names meaningful names.

idanasayag0 · 2024-03-14T12:44:18Z

eyarGdolov/script.js

+            const StorageUsed = document.getElementById('StorageUsed');
+            const StorageUsed1 = document.getElementById('StorageUsed1');
+            StorageUsed.innerHTML = maxSize - MbLeft;
+            StorageUsed1.innerHTML = maxSize - MbLeft;


You can save the calculate in a separate variable instead of doing it twice.

let remainingSize = maxSize - MbLeft StorageUsed.innerHTML = remainingSize StorageUsed1.innerHTML = remainingSize

idanasayag0 · 2024-03-14T12:46:44Z

eyarGdolov/script.js

+    function calcFileSize(file) {
+        if (checkImageSize(file)) {
+            var totalUsedSpaceMb = Math.ceil(files.reduce((acc, curr) => acc + curr.size / (1024 ** 2), 0));
+            const MbLeft = maxSize - totalUsedSpaceMb;


The convention of variable names in JavaScript is camelCase.
you can read about it:
https://developer.mozilla.org/en-US/docs/Glossary/Camel_case

idanasayag0 · 2024-03-14T13:00:16Z

eyarGdolov/script.js

+
+<!-- This function checks the size of the image that the user uploads-->
+    function checkImageSize(file) {
+        const imageSizeMB = ((file.size) / (1024 ** 2));


You should rename the variable from file to image.
You called it a file but treat it as an image.
const imageSizeMb = ((file.size) / (1024 ** 2));

idanasayag0 · 2024-03-14T13:08:17Z

benda/Fylo.js

+let gradientElement;
+let storedSize
+let remainingSize
+let storedFiles=[];


You don't use this variable, you should remove.

idanasayag0 · 2024-03-14T13:09:05Z

benda/Fylo.js

+   storedSize+=sizeInMB;
+//    for(const file of FormatedFiles){
+//        storedFiles.push({name:file.name,size:file.size/1024/1024});
+//    }


Remove unnecessary comments.

idanasayag0 · 2024-03-14T13:20:02Z

benda/Fylo.js

+    }
+    for(const name of fileNames){
+        if(!(name.toLowerCase().endsWith('.jpg')||name.toLowerCase().endsWith('.jpeg')||name.toLowerCase().endsWith('.png')||name.toLowerCase().endsWith('.gif'))){
+            error="one or more of the files isn't supported";


There is a lot ways to solve it in the right way.
for example:

In your way, you repeat name.toLowerCase() 4times.
You should save it in separate variable.

const lowerCaseName = name.toLowerCase() if(lowerCaseName.endsWith('.jpg') || lowerCaseName.endsWith('.jpeg') || lowerCaseName.endsWith('.png') || lowerCaseName.endsWith('.gif'))

You can make it more clean.

... if(isValidImageFileName(lowerCaseName)) ... function isValidImageFileName(fileName) { const regex = /\.(jpg|jpeg|png|gif)$/i; return regex.test(fileName); }

read about regex
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Guide/Regular_expressions

idanasayag0 · 2024-03-14T13:23:41Z

benda/Fylo.js

+
+ function validateFiles(fileNames,sizeOfFiles){ // checking if the files are supported and if there is enough space to upload them
+    let error='';
+    if(sizeOfFiles+storedSize>150){


You should make a global variable instead of 150.
you repeat 150 few times.
const maxSize = 150

alonman17 · 2024-03-14T17:13:42Z

omerAmsalem/functions.js

@@ -0,0 +1,86 @@
+const TotalSizeMb = 100;
+var files = [];


Use const instead
https://www.freecodecamp.org/news/var-let-and-const-whats-the-difference/

GilHeller

Nice job
Generally, use "let" and "const" instead of "var".
Check out my comments.

GilHeller · 2024-03-14T16:41:03Z

barRachmay/script.js

+
+function initializeStorage() {
+    // Check if the storage is already initialized
+    if (!localStorage.getItem('totalStorage')) {


Consider defining totalStorage as a constant

GilHeller · 2024-03-14T16:47:55Z

barRachmay/script.js

+    }
+}
+
+function CheckSpace (fileSize) {


Functions names should start with a lower case letter while classes will start with an upper case letter.
Rename to checkSpace

GilHeller · 2024-03-14T16:55:27Z

barRachmay/script.js

+function updateStorageUI() {
+    var totalStorage = parseInt(localStorage.getItem('totalStorage')); //this makes char to int
+    var usedStorage = parseInt(localStorage.getItem('usedStorage'));
+
+    document.getElementById('remainingStorage').textContent = totalStorage;
+    document.getElementById('usedStorage').textContent = usedStorage;
+
+    var remainingStorage = totalStorage - usedStorage;
+    var percentageUsed = (usedStorage) ;
+    var progressBar = document.getElementById('progressBar');
+    progressBar.style.width = percentageUsed + '%';
+
+}


updateStorageUI has two responsibilities: calculate the percentages and update in the UI.
Functions should have one responsibility only .
Consider to create a new function to calculate the percentages.

GilHeller · 2024-03-14T17:05:14Z

barRachmay/script.js

+function uploadFile() {
+
+    checkFileFormat();
+
+
+}


uploadFile should contain all steps to upload a file:

function uploadFile() { checkFileFormat(); checkSpace(); updateStorageUI() }

alonman17 · 2024-03-14T17:22:26Z

omerAmsalem/functions.js

+
+        if (!allowedExtensions.some(ext => file.name.toLowerCase().endsWith(ext))) {
+            alert("Error: File format is not supported.");
+            console.error('Error: File format is not supported.');


Remove console logs

alonman17 · 2024-03-14T17:27:15Z

omerAmsalem/functions.js

+
+function chooseFile() {
+    console.log('chooseFile');
+    const fileInput = document.createElement('input');


Why creating the HTML element here?

alonman17 · 2024-03-14T17:35:07Z

omerAmsalem/functions.js

+}
+
+function fileSizeCalculator(file) {
+    if(checkFileSize(file)){


Redundant call to checkFileSize()
In addition, functions should only have one porpuse.
fileSizeCalculator should get a file, and output it size

alonman17 · 2024-03-15T11:37:01Z

inonEl/script.js

+    let state = loadState();
+    state.sizeNum = Math.round((state.sizeNum - fileSize) * 100) / 100;
+
+    state.files = state.files.filter(file => file.name !== button.textContent);


I Would give an id to the button.
Searching base on the textContent is bad practice

alonman17 · 2024-03-15T11:37:17Z

inonEl/script.js

+
+
+function handleRemove(button, fileSize) {
+    let state = loadState();


const here aswell

Tamir198 · 2024-03-15T11:37:25Z

omerElias/index.html

+    var currentSize = 0;
+
+if (localStorage.getItem('current_size') !== null) {
+    currentSize = Number(localStorage.getItem('current_size'));


You can save localStorage.getItem('current_size') into a variable to prevent code duplication

alonman17 · 2024-03-15T11:37:48Z

inonEl/script.js

+
+
+document.addEventListener('DOMContentLoaded', function () {
+    let state = loadState();


Tamir198 · 2024-03-15T11:38:15Z

omerElias/index.html

+
+function updateUI() {
+    var left_mb = 800 - currentSize;
+    document.getElementById('current_mb_text').textContent = currentSize + ' MB';


Please dont use magic numbers, save 800 into a variable for example const MAX_SIZE = 800 and use the variable.
It will be more readable.

Also be consistent with teh naming of your variables (picke one style for example variable_name or variableName and stick to it)

alonman17 · 2024-03-15T11:38:28Z

inonEl/script.js

+});
+
+
+function clearState() {


unused function

Tamir198 · 2024-03-15T11:38:54Z

omerElias/index.html

+    document.getElementById('current_mb_text').textContent = currentSize + ' MB';
+    document.getElementById('current_mb_title').textContent = currentSize + ' MB';
+    document.getElementById('left_mb').textContent = left_mb + ' MB';
+


Also you can save the MB into a variable as well

Tamir198 · 2024-03-15T11:40:52Z

omerElias/index.html

+    var fileSize = e.target.files[i].size;
+    var fileType = e.target.files[i].type;
+    var fileSizeinMb = Math.floor(fileSize / 1000);
+


If you choose to use camle case use caps on every word - fileSizeInMb
This calculation is not correct, I uploaded a file with a size of 811KB and the variable fileSizeinMb was 800.

alonman17 · 2024-03-15T11:43:04Z

inonEl/script.js

+                updateLeft(state.sizeNum);
+
+                const filesDiv = document.getElementById("files");
+                const button = document.createElement('button');


Can seperate to another function.
createFileButton()

alonman17 · 2024-03-15T11:47:18Z

inonEl/script.js

@@ -0,0 +1,112 @@
+function saveState(sizeNum, files) {


Very well done overall!

GilHeller · 2024-03-15T11:36:13Z

liorHassin/script.js

+
+async function uploadFiles(){
+    let size = 0;
+    const filesSystemFileHandler = await showOpenFilePicker({multiple: true});


It can be done with the html: <input type="file" multiple>

GilHeller · 2024-03-15T11:37:35Z

liorHassin/script.js

+    const files = await extractFileFromHandler(filesSystemFileHandler);
+
+    files.forEach(element => {
+        size+= element.size/1024/1024;


Consider saving the size of MB as a constant.

GilHeller · 2024-03-15T11:47:43Z

liorHassin/script.js

+async function uploadFiles(){
+    let size = 0;
+    const filesSystemFileHandler = await showOpenFilePicker({multiple: true});
+    const files = await extractFileFromHandler(filesSystemFileHandler);
+
+    files.forEach(element => {
+        size+= element.size/1024/1024;
+    });
+
+    const error = validateFiles(files, size);
+    if(error !== "") {
+        alert(error);
+        return;
+    }
+
+    storedSize += size;
+    files.forEach(file => {storedFiles.push({fileName: file.name, fileSize: file.size/1024/1024});});
+
+    updateView();
+    localStorage.setItem('storedSize', storedSize);
+}


Nice, but the function should be attached to an input element.

It is a bad practice to get input from an element that doesn't have input functionality.

See the example here: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input/file

GilHeller · 2024-03-15T12:15:40Z

morAlmakayes/program.js

+
+const FillTheBar = document.getElementById('FillTheBarId');
+
+document.getElementById('fileInput').onchange = function() {


Nice, but this may cause errors on a large scale.
By defining onchange = function() {...} you override the existing/default function.
The best practice is to use element.addEventListener("change", function(e){...})

See: https://developer.mozilla.org/en-US/docs/Web/API/EventTarget/addEventListener

GilHeller · 2024-03-15T12:17:34Z

morAlmakayes/program.js

+
+    for (let i = 0; i < files.length; i++) {
+        const file = files[i];
+        totalFileSize += file.size / 1048576; 


Consider saving the size of MB in a constant

const MB = 1048576 totalFileSize += file.size / MB;

GilHeller · 2024-03-15T12:18:57Z

morAlmakayes/program.js

+        usedSpaceId.textContent = usedSpace.toFixed(2); 
+        availableDiskSpaceId.textContent = availableDiskSpace.toFixed(2)+ ' MB ' ; 
+        FillTheBar.style.width = '0%'; 
+    }לא


GilHeller · 2024-03-15T12:26:03Z

morAlmakayes/program.js

+    if (usedSpace + totalFileSize <= totalDiskSpace) {
+        usedSpace += totalFileSize;
+        availableDiskSpace = totalDiskSpace - usedSpace;
+
+        usedSpaceId.textContent = usedSpace.toFixed(2);
+        availableDiskSpaceId.textContent = availableDiskSpace.toFixed(2)+ ' MB ' ;
+
+        FillTheBar.style.width = (usedSpace * 100) / totalDiskSpace + '%';
+
+    } else {
+
+        alert('There is not enough space on the disk, please try again');
+        document.getElementById('fileInput').value = ''; 
+        usedSpace = 0; 
+        availableDiskSpace = totalDiskSpace; 
+        usedSpaceId.textContent = usedSpace.toFixed(2); 
+        availableDiskSpaceId.textContent = availableDiskSpace.toFixed(2)+ ' MB ' ; 
+        FillTheBar.style.width = '0%'; 


There is repetition.
Do all calculations inside the if-else statement. Then, apply the results to the elements.

let barWidth = 0 if (usedSpace + totalFileSize <= totalDiskSpace) { usedSpace += totalFileSize; availableDiskSpace = totalDiskSpace - usedSpace; barWidth = (usedSpace * 100) / totalDiskSpace } else { alert('There is not enough space on the disk, please try again'); document.getElementById('fileInput').value = ''; usedSpace = 0; availableDiskSpace = totalDiskSpace; } usedSpaceId.textContent = usedSpace.toFixed(2); availableDiskSpaceId.textContent = availableDiskSpace.toFixed(2)+ ' MB ' ; FillTheBar.style.width = `${barWidth}%`;

BarSh0 · 2024-03-15T14:02:54Z

amosZohar/index.html

+                </div>
+                <div class="icon-img">
+                    <img  src="images/icon-upload.svg" alt="" aria-hidden="true" />
+                </div>>


remove the '>' char

BarSh0 · 2024-03-15T14:04:22Z

amosZohar/index.html

+    <script src="index.js"></script>
+</head>
+
+<body onload="Initialize()">


Nice using of onload

ozizhakGT

left you some comments
Great work! keep going 🚀

ozizhakGT · 2024-03-15T14:45:27Z

yoavFridman/index.html

+                <div class="icon-img" id="uploadIcon" onclick="openUploadFileDialog()">
+                    <img src="images/icon-upload.svg" alt="" aria-hidden="true" />
+                    <input type="file" id="fileUpload" style="display: none;" />
+                </div>


using <label> element with for attribute, could prevent us for create unnecessary event handler and connect the label element to your input element
https://developer.mozilla.org/en-US/docs/Web/HTML/Element/label

ozizhakGT · 2024-03-15T14:49:39Z

yoavFridman/index.js

+function openUploadFileDialog() {
+    var fileUpload = document.getElementById('fileUpload');
+    fileUpload.click();
+    fileUpload.addEventListener('change', handleNewFile);


it's great to create event listeners, it's much greater to remove the kill event listener when you finish with them
please read about removeEventListener method and understand how much it important to clean events!

ozizhakGT · 2024-03-15T14:51:20Z

yoavFridman/index.js

+function changeStats() {
+    document.getElementById('storage-span').innerHTML = storageUsed + ' GB';
+    document.getElementById('gb-left-span').innerHTML = maxStorage + ' ' + '<span>GB left</span>';
+    document.getElementById('gradient-bar').style.width = (storageUsed / 999) * 100 + '%';


Suggested change

document.getElementById('gradient-bar').style.width = (storageUsed / 999) * 100 + '%';

document.getElementById('gradient-bar').style.width = (storageUsed / maxStorage) * 100 + '%';

great practice to create and assign constants variable like maxStorage, but why not using them? :)

ozizhakGT · 2024-03-15T14:59:35Z

yoavFridman/index.js

+var storageUsed = 0;
+var maxStorage = 999;


var keyword not used anymore and can make trouble
for that es6 bring with him let and const keywords.

please read about block scopes VS funtcional scopes
https://blog.coolhead.in/difference-between-function-scope-and-block-scope-in-javascript

ozizhakGT

left you some comments
Great work! keep going 🚀

ozizhakGT · 2024-03-15T15:08:26Z

rayNaftali/main.js

+var totalDiskSpace = 10; // in MB
+var currentSpace = 0;
+var files = {}; // Store file names and their sizes
+var allowedFormats = ['.jpg', '.jpeg', '.gif', '.png']; // Allowed file formats


var keyword not used anymore and not relevant
also it can make us a lots of trouble when we thinking about scopes
es6 brought to us the let/const keywords for replacement

please read Block Scope VS Functional Scopes -
https://blog.coolhead.in/difference-between-function-scope-and-block-scope-in-javascript

ozizhakGT · 2024-03-15T15:09:44Z

rayNaftali/main.js

+
+    input.addEventListener('change', function(event) {
+        var newFiles = event.target.files;
+        for (var i = 0; i < newFiles.length; i++) {


the var inside the for loop very bed practice

Suggested change

for (var i = 0; i < newFiles.length; i++) {

for (let i = 0; i < newFiles.length; i++) {

ozizhakGT · 2024-03-15T15:11:31Z

rayNaftali/main.js

+            } else {
+                alert('File "' + fileName + '" is already uploaded.');
+            }
+        }
+
+        // Save files localStorage after processing all files
+        saveStateToLocalStorage();


by trigger this alerts above, do you prevent the save? please check it
it seems you still add the file

ozizhakGT · 2024-03-15T15:16:48Z

rayNaftali/main.js

+function loadStateFromLocalStorage() {
+    var savedFiles = localStorage.getItem('files');
+    if (savedFiles !== null) {
+        files = JSON.parse(savedFiles);


files should be variable?

ozizhakGT · 2024-03-15T15:18:29Z

rayNaftali/main.js

+    removeText.addEventListener('click', function() {
+        listItem.remove();
+        var fileSizeMB = files[fileName];
+        delete files[fileName];
+        updateAvailableSpace(-fileSizeMB);
+        saveStateToLocalStorage(); // Update localStorage after removing file
+    });


please create for this function and call it instead of write it as anonymous function

Suggested change

removeText.addEventListener('click', function() {

listItem.remove();

var fileSizeMB = files[fileName];

delete files[fileName];

updateAvailableSpace(-fileSizeMB);

saveStateToLocalStorage(); // Update localStorage after removing file

});

removeText.addEventListener('click', onRemoveFile);

another thing, we must clean after our events then not listening anymore!!
please read about removeEventListener method

ozizhakGT

left you some comments
Great work! keep going 🚀

ozizhakGT · 2024-03-15T15:26:19Z

ofekWorcel/index.html

+                <div class="icon-img">
+                    <img src="images/icon-document.svg" alt="" aria-hidden="true" />
+                </div>
+                <div class="icon-img" id="imageUploader" alt="Upload Files" style="cursor: pointer;" onclick="document.getElementById('fileInput').click();">


it very bad practice expose your ids and your events, it can bring a lots of hackers to scan your websites and business.

Suggested change

<div class="icon-img" id="imageUploader" alt="Upload Files" style="cursor: pointer;" onclick="document.getElementById('fileInput').click();">

<div class="icon-img" id="imageUploader" alt="Upload Files" style="cursor: pointer;" onclick="onUploadFiles()">

ozizhakGT · 2024-03-15T15:28:32Z

ofekWorcel/index.html

+                <div class="icon-img" id="imageUploader" alt="Upload Files" style="cursor: pointer;" onclick="document.getElementById('fileInput').click();">
+                    <img src="images/icon-folder.svg" alt="" aria-hidden="true" />
+                    <!-- Hidden file input -->
+                    <input type="file" id="fileInput" multiple accept=".jpg, .jpeg, .gif, .png" style="display: none;" onchange="handleFiles(this.files)"> 


ozizhakGT · 2024-03-15T15:29:13Z

ofekWorcel/scripts.js

+var minMemory = "0";
+var maxMemory = "10 MB"
+var currentDiskSpace = 0;
+var diskSize = 10 * 1024 * 1024; // 10 MB for testing
+var currentPercent = 0;


var keyword not used anymore and not relevant
also it can make us a lots of trouble when we thinking about scopes
es6 brought to us the let/const keywords for replacement

please read Block Scope VS Functional Scopes -
https://blog.coolhead.in/difference-between-function-scope-and-block-scope-in-javascript

ozizhakGT · 2024-03-15T15:33:44Z

ofekWorcel/scripts.js

+    function createFilePreview(file, fileId) {
+
+        var fileName = file.name;
+        var fileExtension = fileName.slice(((fileName.lastIndexOf(".") - 1) >>> 0) + 2);


can you explain what the >>> operator do?

ozizhakGT · 2024-03-15T15:35:14Z

ofekWorcel/scripts.js

+    if (currentDiskSpace + totalSize > diskSize) {
+        alert('There is not enough space on the disk.');
+    } else {
+    Array.from(files).forEach(function(file, index) {


great implantation using Array.from for converting ArrayLike to Array!
I love it! 🚀

ozizhakGT · 2024-03-15T15:36:58Z

ofekWorcel/scripts.js

+function updateHTML(elementName, value){
+    document.getElementById(elementName).innerHTML = value;  
+}   
+var i = 0;


where this i variable used?
can be problem when it lives in global

ozizhakGT · 2024-03-15T15:39:35Z

ofekWorcel/scripts.js

+        filePreviewContainer.appendChild(filePreview);
+
+
+        deleteIndicator.addEventListener('click', function() {


creating event listener and remove them when we don't need them can make our application preform very bad and it ca lead to stackoverflow problem and crash the browser

please read about removeEventListener method

ozizhakGT

left you some comments
Great work! keep going 🚀

ozizhakGT · 2024-03-15T15:45:09Z

noaVinkler/index.html

+                    <label for="choose-file" >
+                        <img src="images/icon-folder.svg" alt="" aria-hidden="true" />
+                    </label>
+                    <input type="file" id="choose-file" onchange="onFileInputChange(this)" style="display: none;" />


this keyword are not used anymore and bed practice in this point of time
event function will bring you out of the box the event data as first argument

please read about createEventListener method

ozizhakGT · 2024-03-15T15:46:29Z

noaVinkler/index2.js

+  progressElement.style.width = percent.toString(10) + '%';
+};
+
+const addCurrentFileSize = (s) => {


what s means?
naming conversion is very critical in our world!

ozizhakGT · 2024-03-15T15:49:56Z

noaVinkler/index2.js

+  removeButton.addEventListener('click',()=>{
+    span.remove();
+    addCurrentFileSize(-size);
+  });


for that you can create new function called onRemoveFile

Suggested change

removeButton.addEventListener('click',()=>{

span.remove();

addCurrentFileSize(-size);

});

removeButton.addEventListener('click', onRemoveFile);

another thing
creating event listener and remove them when we don't need them can make our application preform very bad and it can lead to stackoverflow problem and crash the browser

please read about removeEventListener method

ozizhakGT · 2024-03-15T15:52:01Z

noaVinkler/index2.js

+  if (usedSize + s < totalSize) {
+    usedSize += s;
+    sizeLeft = totalSize - usedSize;
+    percent = 100 * (usedSize / totalSize) - 4;
+    usedSizeElement.innerText = shortenSizeNumber(usedSize) + ' MB';
+    sizeLeftElement.innerText = shortenSizeNumber(sizeLeft);
+    progressElement.style.width = percent.toString(10) + '%';
+    progressElement.style.transition = 'width 0.5s ease 0.1s';
+  } else {
+    alert('There is not enough space on the disk');
+  }


very good approach using the if / else statement and prevent uplaod file!
love it 🚀

ozizhakGT · 2024-03-15T15:53:37Z

noaVinkler/index2.js

index2.js as file name can be not so good
try renaming as main.js

ozizhakGT · 2024-03-15T15:54:58Z

noaVinkler/index.html

+                    <label for="choose-file" >
+                        <img src="images/icon-folder.svg" alt="" aria-hidden="true" />
+                    </label>
+                    <input type="file" id="choose-file" onchange="onFileInputChange(this)" style="display: none;" />


I see you use the power of the label element sticking with input element
I love it! 🚀

finished Oz CRs

ozizhakGT

left you some comments
Great work! keep going 🚀

ozizhakGT · 2024-03-16T17:43:50Z

orenLevi/index.html

+            <img class="logo" src="images/logo.svg" alt="Fylo logo" />
+            <section class="icons">
+                <div class="icon-img">
+                    <button id = "documentButton" class="imageDocumentButton">


try to prevent spaces in elements attributes

Suggested change

<button id = "documentButton" class="imageDocumentButton">

<button id="documentButton" class="imageDocumentButton">

ozizhakGT · 2024-03-16T17:44:31Z

orenLevi/index.html

+                </div>
+                <div class="icon-img">
+                    <button onclick="importData()" class="imageUpladFile">
+                    <img src="images/icon-folder.svg" alt="" aria-hidden="true" />


always put value for alt attribute!

ozizhakGT · 2024-03-16T17:47:18Z

orenLevi/project2.js

+document.addEventListener('DOMContentLoaded', function() {
+    const documantButton = document.getElementById('documentButton');
+    const gradientBar = document.getElementById('gradientBar');
+    const sizeText = document.getElementById('sizeText');
+    const remaningSize = document.getElementById('remaningSize');
+    const fileNamesContainer = document.getElementById('fileNamesContainer');
+    const restMem = document.getElementById('restMem');
+    getLocalStorage();
+});


you sure it work for you?
please read the compare between block scope VS functional scope

ozizhakGT · 2024-03-16T17:49:46Z

orenLevi/project2.js

+let totalSizeInMB = 0;
+
+function getLocalStorage() { // the function get the file how save in the local storage
+    var tmpdata = JSON.parse(localStorage.getItem('fileStorage'));


var keyword not used anymore, instead use const
also it can make you some trouble when it inside function decleration.

ozizhakGT · 2024-03-16T17:50:59Z

orenLevi/project2.js

+    remaningSize.textContent = (10 - totalSizeInMB).toFixed(2) + ' MB';
+    restMem.textContent = totalSizeInMB.toFixed(2) + ' MB';


Suggested change

remaningSize.textContent = (10 - totalSizeInMB).toFixed(2) + ' MB';

restMem.textContent = totalSizeInMB.toFixed(2) + ' MB';

remaningSize.innerText = (10 - totalSizeInMB).toFixed(2) + ' MB';

restMem.innerText = totalSizeInMB.toFixed(2) + ' MB';

ozizhakGT · 2024-03-16T17:54:37Z

orenLevi/project2.js

+        let files = Array.from(input.files);
+        files.forEach(function(file) {
+            let fileSizeInMB = file.size / (1024 * 1024);
+            if (fileSizeInMB > 10) {


consider create constant for the TOTAL_MAX_SIZE instead of change this parameter hard coded
for example if I ask to increase the max size of your hardware, you will start scan all you project and changing the 10 parameter, but if you create constant variable you will just changing it and that all

ozizhakGT · 2024-03-16T17:56:46Z

orenLevi/project2.js

+    fileNameElement.addEventListener('click', function() {
+        fileNamesContainer.removeChild(fileNameElement);
+        removeFile(fileSizeInMB);
+        localStorage.removeItem('fileStorage');
+    });


creating event listeners and not remove them when we don't need them can make our application preform very bad and it can lead to stackoverflow problem and crash the browser

please read about removeEventListener method

ozizhakGT · 2024-03-16T17:58:24Z

orenLevi/project2.js

+    let fileNameElement = document.createElement('button');
+    fileNameElement.classList.add('file-button');
+    fileNameElement.textContent = fileName;
+    fileNameElement.addEventListener('click', function() {


consider create function and pass it in the event listener instead of creating anonymos functon

Suggested change

fileNameElement.addEventListener('click', function() {

fileNameElement.addEventListener('click', onRemoveFile)

danielmargolin · 2024-03-15T11:32:33Z

shirYahav/index.js

+function moveBar(size) {
+    const element = document.getElementById("gradient-bar-id");
+    let currentWidth = parseFloat(element.style.width) || 3;
+    let targetWidth = size * 10;


can be const

danielmargolin · 2024-03-15T11:36:27Z

shirYahav/index.js

+}
+
+//Get total size from local storage
+function getTotalFileSize() {


const getTotalFileSize = () => parseFloat(localStorage.getItem('totalFileSize')) || 0;

danielmargolin · 2024-03-16T19:59:42Z

shirYahav/index.js

+
+const fileInput = document.getElementById('fileInput');
+const fileSizeDisplay = document.getElementById('fileSizeDisplay');
+const MBLeftElement = document.getElementById('MBLeft');


BarSh0 · 2024-03-15T14:04:46Z

amosZohar/index.js

@@ -0,0 +1,178 @@
+/*NOTE: the size of the files extracted from the <input type="file"> element


The storage number in the front dont work.
You've used NaN Bytes of your storage

BarSh0 · 2024-03-15T14:11:31Z

amosZohar/index.html

+        </section>
+        <!-- Storage Container -->
+        <section class="storage-container">
+            <h1>You've used <span id="used-space">815 GB</span> of your storage</h1>


If JavaScript updates this number, don't display a number here; instead, display "loading..." for example.

BarSh0 · 2024-03-15T14:13:31Z

amosZohar/index.js

+        if(key.split(size_suffix).length == 1 && key !== AVAIL_SPACE ){
+            append_file_name(key);
+        }
+    }


There is no need for comments within a function, clean and readable code speaks for itself.

BarSh0 · 2024-03-15T14:21:05Z

amosZohar/index.js

+        if(key.split(size_suffix).length == 1 && key !== AVAIL_SPACE ){
+            append_file_name(key);
+        }
+    }


In this case, it's preferable to use (!localStorage.getItem(AVAIL_SPACE)) instead.

BarSh0 · 2024-03-15T14:23:36Z

amosZohar/index.js

+const openUploadMenu = e => document.getElementById("upload-input").click();
+
+const supported_extensions = ["jpg","jpeg","png","gif","JPG","JPEG","PNG","GIF"];
+


Where does this const belong? If it's for use throughout the entire file, place it at the top of the file. If it's only relevant to a specific function, put it at the top of that function.

BarSh0 · 2024-03-16T16:59:50Z

shirYahav/index.js

@@ -0,0 +1,82 @@
+


the number in the left side of the proggress bar dont change

BarSh0 · 2024-03-16T17:00:30Z

shirYahav/index.js

+    }  
+}
+
+//Get total size from local storage


There is no need for comments within a function, clean and readable code speaks for itself.

BarSh0 · 2024-03-16T17:17:29Z

shirYahav/index.js

+//Update progress bar based on total file size
+function updateProgressBar(totalSize, previousSize) {
+    moveBar(totalSize, previousSize);
+    fileSizeDisplay.textContent = totalSize === 0 ? totalSize.toFixed(0) : totalSize.toFixed(2);


name values to make the code more readable, and reuse this const:

moveBar(totalSize, previousSize); const totalSizeValue = totalSize === 0 ? totalSize.toFixed(0) : totalSize.toFixed(2); fileSizeDisplay.textContent = totalSizeValue; MBLeftElement.firstChild.textContent = 10 - totalSizeValue;

BarSh0 · 2024-03-16T17:19:31Z

shirYahav/index.js

+        return;
+    }
+
+    const fileSize = (selectedFile.size / (1024 * 1024));


if u are gonna use a lot in files size make a consts in the top of the code

const GB = 1024 * 1024 * 1024; const MB = 1024 * 1024; const KB = 1024;

BarSh0 · 2024-03-16T17:23:38Z

shirYahav/index.js

+const fileInput = document.getElementById('fileInput');
+const fileSizeDisplay = document.getElementById('fileSizeDisplay');
+const MBLeftElement = document.getElementById('MBLeft');
+


There is no limit to the types of files that can be uploaded

add all files

61e7d36

danielmargolin commented Mar 13, 2024

View reviewed changes

alonman17 reviewed Mar 13, 2024

View reviewed changes

idanasayag0 and others added 11 commits March 14, 2024 00:15

.

96b36a2

.

3ee28eb

.

c398ea6

.

b9f092b

.

94dc553

.

f17982c

Update README.md

221f0b4

Update README.md

bed053f

Update README.md

9f075e6

.

7931232

Merge branch 'fyloCR' of https://github.com/ColmanDevClub/FyloCodeReview

692355e

into fyloCR

idanasayag0 reviewed Mar 14, 2024

View reviewed changes

idanasayag0 and others added 2 commits March 14, 2024 15:01

Update README.md

d4b4678

.

75af4e5

idanasayag0 reviewed Mar 14, 2024

View reviewed changes

idanasayag0 and others added 4 commits March 14, 2024 15:26

Update README.md

c2f1060

.

b1ce200

Update README.md

a86be5a

Update README.md

6c5ade7

alonman17 reviewed Mar 14, 2024

View reviewed changes

GilHeller reviewed Mar 14, 2024

View reviewed changes

Update README.md

2a08027

alonman17 reviewed Mar 14, 2024

View reviewed changes

alonman17 and others added 3 commits March 14, 2024 19:41

readme

a871d98

.

005fc8d

Update README.md

5284aa6

.

46deff2

alonman17 reviewed Mar 15, 2024

View reviewed changes

inonEl/script.js

function handleRemove(button, fileSize) {

let state = loadState();

Copy link

alonman17 Mar 15, 2024

Choose a reason for hiding this comment

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

const here aswell

Tamir198 reviewed Mar 15, 2024

View reviewed changes

alonman17 reviewed Mar 15, 2024

View reviewed changes

inonEl/script.js

document.addEventListener('DOMContentLoaded', function () {

let state = loadState();

Copy link

alonman17 Mar 15, 2024

Choose a reason for hiding this comment

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

const

Tamir198 reviewed Mar 15, 2024

View reviewed changes

alonman17 reviewed Mar 15, 2024

View reviewed changes

inonEl/script.js

});

function clearState() {

Copy link

alonman17 Mar 15, 2024

Choose a reason for hiding this comment

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

unused function

Tamir198 reviewed Mar 15, 2024

View reviewed changes

alonman17 reviewed Mar 15, 2024

View reviewed changes

inonEl/script.js

@@ -0,0 +1,112 @@

function saveState(sizeNum, files) {

Copy link

alonman17 Mar 15, 2024

Choose a reason for hiding this comment

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

Very well done overall!

alonman17 and others added 2 commits March 15, 2024 13:47

readme

f3ae26f

Omer Elias

ab68d1a

GilHeller reviewed Mar 15, 2024

View reviewed changes

Update README.md

f5f7136

GilHeller reviewed Mar 15, 2024

View reviewed changes

Update README.md

48157b3

BarSh0 reviewed Mar 15, 2024

View reviewed changes

ozizhakGT requested changes Mar 15, 2024

View reviewed changes

ozizhakGT reviewed Mar 15, 2024

View reviewed changes

ozizhakGT and others added 3 commits March 15, 2024 17:59

Update README.md

604a830

finished Oz CRs

Update README.md

dde49b4

Update README.md

c6e6cc5

ozizhakGT reviewed Mar 16, 2024

View reviewed changes

danielmargolin commented Mar 16, 2024

View reviewed changes

BarSh0 reviewed Mar 18, 2024

View reviewed changes

		@@ -0,0 +1,175 @@
		//formatting bytes to MB/KB/GB for UI.
		function formatSizeUnits(fileSize) {



		function handleRemove(button, fileSize) {
		let state = loadState();



		document.addEventListener('DOMContentLoaded', function () {
		let state = loadState();


		const FillTheBar = document.getElementById('FillTheBarId');

		document.getElementById('fileInput').onchange = function() {

	document.getElementById('gradient-bar').style.width = (storageUsed / 999) * 100 + '%';
	document.getElementById('gradient-bar').style.width = (storageUsed / maxStorage) * 100 + '%';

	for (var i = 0; i < newFiles.length; i++) {
	for (let i = 0; i < newFiles.length; i++) {

	<div class="icon-img" id="imageUploader" alt="Upload Files" style="cursor: pointer;" onclick="document.getElementById('fileInput').click();">
	<div class="icon-img" id="imageUploader" alt="Upload Files" style="cursor: pointer;" onclick="onUploadFiles()">

		filePreviewContainer.appendChild(filePreview);


		deleteIndicator.addEventListener('click', function() {

	<button id = "documentButton" class="imageDocumentButton">
	<button id="documentButton" class="imageDocumentButton">

		remaningSize.textContent = (10 - totalSizeInMB).toFixed(2) + ' MB';
		restMem.textContent = totalSizeInMB.toFixed(2) + ' MB';

	fileNameElement.addEventListener('click', function() {
	fileNameElement.addEventListener('click', onRemoveFile)

		@@ -0,0 +1,178 @@
		/*NOTE: the size of the files extracted from the <input type="file"> element

		const openUploadMenu = e => document.getElementById("upload-input").click();

		const supported_extensions = ["jpg","jpeg","png","gif","JPG","JPEG","PNG","GIF"];

add all files #1

Are you sure you want to change the base?

add all files #1

Conversation

danielmargolin commented Mar 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alonman17 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GilHeller left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GilHeller Mar 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tamir198 Mar 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tamir198 Mar 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ozizhakGT left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alonman17 left a comment •

edited

Loading

GilHeller Mar 14, 2024 •

edited

Loading

Tamir198 Mar 15, 2024 •

edited

Loading

Tamir198 Mar 15, 2024 •

edited

Loading