Bugfix/mathilde - #24
Conversation
…de ressembler au Timer
…ion en setInterval()
…point-virgule et indentation du code
…déjà présents dans le CSS
There was a problem hiding this comment.
Pull Request Overview
This PR implements comprehensive bug fixes for a Timer application, correcting critical HTML, CSS, and JavaScript errors to make the code functional and improve readability.
- Fixed HTML structure and syntax errors including file extension, malformed tags, and duplicate IDs
- Corrected CSS selector typos and property values that prevented proper styling
- Resolved JavaScript functionality issues including incorrect API usage, missing parameters, and logical errors
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| index.php | Removed original malformed HTML file |
| index.html | Created properly structured HTML with corrected syntax and file paths |
| assets/js/script.js | Fixed JavaScript errors including API names, missing parameters, and logical issues |
| assets/css/style.css | Corrected CSS selectors, property values, and improved code formatting |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| @@ -1,32 +1,32 @@ | |||
| window.addEventListener("DOMContentLoaded", (event) => { | |||
| let is_run = "true"; | |||
| window.addEventListener("DOMContentLoaded", (event) => { //event ? | |||
There was a problem hiding this comment.
Corrected spelling of 'évènement' to 'événement' in the French comment.
| document.querySelector('#hours').innerHTML = adjustTimer(oDate.getHours()); | ||
| document.querySelector('#minutes').innerHTML = adjustTimer(oDate.getMinutes()); | ||
| document.querySelector('#seconds').innerHTML = adjustTimer(oDate.getSeconds()); |
There was a problem hiding this comment.
Using innerHTML for plain text content is unnecessary and potentially unsafe. Use textContent instead since no HTML markup is being inserted.
| document.querySelector('#hours').innerHTML = adjustTimer(oDate.getHours()); | |
| document.querySelector('#minutes').innerHTML = adjustTimer(oDate.getMinutes()); | |
| document.querySelector('#seconds').innerHTML = adjustTimer(oDate.getSeconds()); | |
| document.querySelector('#hours').textContent = adjustTimer(oDate.getHours()); | |
| document.querySelector('#minutes').textContent = adjustTimer(oDate.getMinutes()); | |
| document.querySelector('#seconds').textContent = adjustTimer(oDate.getSeconds()); |
There was a problem hiding this comment.
C'est juste. Cela permet les injections de code.
| function randomHexColor(x, y){ | ||
| return "rgb(" + Math.floor(x/100 * 256) + "," + Math.floor(y/100 * 256) + "," + Math.floor(z/100 * 256) + ")"; | ||
| function randomHexColor(x, y, z) { | ||
| return "rgb(" + Math.floor(x / 100 * 256) + "," + Math.floor(y / 100 * 256) + "," + Math.floor(z / 100 * 256) + ")"; // pourquoi ne pas convertir en nombre les chaines de caractères x, y et z ? |
There was a problem hiding this comment.
The parameters x, y, z are strings from innerHTML but are used in mathematical operations without conversion. Use parseInt() or Number() to convert them to numbers first.
| return "rgb(" + Math.floor(x / 100 * 256) + "," + Math.floor(y / 100 * 256) + "," + Math.floor(z / 100 * 256) + ")"; // pourquoi ne pas convertir en nombre les chaines de caractères x, y et z ? | |
| x = Number(x); | |
| y = Number(y); | |
| z = Number(z); | |
| return "rgb(" + Math.floor(x / 100 * 256) + "," + Math.floor(y / 100 * 256) + "," + Math.floor(z / 100 * 256) + ")"; |
There was a problem hiding this comment.
Il a raison aussi. Javascript type dynamiquement les variables mais on sait jamais... Il vaut mieux être certain de ce qu'on lui donne en paramètre
|
|
||
| init(); | ||
|
|
||
| function init(){ | ||
| document.querySelector('.button').addEventListener('clic', (event) => | ||
| is_run = !is_run | ||
| ((' ' + document.querySelector('.button').className + ' ').indexOf('pause') > -1) ? dcument.querySelector('.button').setAttribute('class', document.querySelector('.button').getAttribute('class').replace(' pause', '')) : document.querySelector('.button').setAttribute('class', document.querySelector('.button').getAttribute('class')+' pause'); | ||
| }) | ||
| function init() { | ||
| document.querySelector('.button').addEventListener('click', (event) => { | ||
| is_run = !is_run; | ||
| ((' ' + document.querySelector('.button').className + ' ').indexOf('pause') > -1) ? document.querySelector('.button').setAttribute('class', document.querySelector('.button').getAttribute('class').replace(' pause', '')) : document.querySelector('.button').setAttribute('class', document.querySelector('.button').getAttribute('class') + ' pause'); // pas mieux de stocker bouton dans une variable ? | ||
| }); | ||
|
|
||
| setTimeInterval(function(){ | ||
| if(is_run){ | ||
| setInterval(function () { | ||
| if (is_run) { | ||
| let oDate = new Date(); | ||
| document.querySelector('#hours').innerHTML = adjustTimer(oDate.getHours()); | ||
| document.querySelector('#minutes').innerHTML = adjustTimer(oDate.getMinutes()); | ||
| document.querySelector('#seconds').innerHTML = adjustTimer( oDate.getSeconds()); | ||
| document.querySelector('#hours').innerHTML = adjustTimer(oDate.getHours()); | ||
| document.querySelector('#minutes').innerHTML = adjustTimer(oDate.getMinutes()); | ||
| document.querySelector('#seconds').innerHTML = adjustTimer(oDate.getSeconds()); | ||
|
|
||
| document.querySelector('body').style.background = randomHexColor(document.querySelector('#hours').innerHTML, document.querySelector('#minutes').innerHTML, document.querySelector('#seconds').innerHTML); | ||
| } | ||
| }, 1000; | ||
| document.querySelector('body').style.background = randomHexColor(document.querySelector('#hours').innerHTML, document.querySelector('#minutes').innerHTML, document.querySelector('#seconds').innerHTML); // innerHTML ne devrait pas être remplacé par textContent vu que ce n'est pas du HTML ? | ||
|
|
||
| } // pourquoi ne pas arrêter la fonction setInterval quand sur pause ? |
There was a problem hiding this comment.
The setInterval continues running even when paused, performing unnecessary DOM queries and date operations. Consider clearing the interval when paused and restarting it when resumed.
There was a problem hiding this comment.
Bon ça. Laisse tomber vu la légèreté du code, inutile de faire du zèle dans l'optimisation
| document.querySelector('.button').addEventListener('click', (event) => { | ||
| is_run = !is_run; | ||
| ((' ' + document.querySelector('.button').className + ' ').indexOf('pause') > -1) ? document.querySelector('.button').setAttribute('class', document.querySelector('.button').getAttribute('class').replace(' pause', '')) : document.querySelector('.button').setAttribute('class', document.querySelector('.button').getAttribute('class') + ' pause'); // pas mieux de stocker bouton dans une variable ? |
There was a problem hiding this comment.
Multiple calls to document.querySelector('.button') are inefficient. Store the button element in a variable to avoid repeated DOM queries.
| document.querySelector('.button').addEventListener('click', (event) => { | |
| is_run = !is_run; | |
| ((' ' + document.querySelector('.button').className + ' ').indexOf('pause') > -1) ? document.querySelector('.button').setAttribute('class', document.querySelector('.button').getAttribute('class').replace(' pause', '')) : document.querySelector('.button').setAttribute('class', document.querySelector('.button').getAttribute('class') + ' pause'); // pas mieux de stocker bouton dans une variable ? | |
| const button = document.querySelector('.button'); | |
| button.addEventListener('click', (event) => { | |
| ((' ' + button.className + ' ').indexOf('pause') > -1) ? button.setAttribute('class', button.getAttribute('class').replace(' pause', '')) : button.setAttribute('class', button.getAttribute('class') + ' pause'); // pas mieux de stocker bouton dans une variable ? | |
| is_run = !is_run; |
Cette pull request propose une correction générale des erreurs HTML, CSS et JS du Timer. Les corrections et les améliorations du code rendent le code fonctionnel et plus lisible.
index.html:index.php->index.html<title>-></title><body>-></body>id="wrapper"lang="fr"deferpour chargement du JS après le HTMLasset->assetsstyle.css:.inside#wrappr->#wrappertranspent->transparentscript.js:let is_run = trueen boléendcument->documentetclic->clicksetTimeIntervalparsetIntervalet fermeture de la fonctionreturndans la fonctionadjustTimerrandomHexColorCommentaires :
innerHTMLpartextContentsetIntervalquand le Timer est sur pauseeventTests réalisés :
Objectifs réalisés :