Sudoku review #1

Open
BordedDev wants to merge 19 commits from BordedDev/sudoku:main into main
pull from: BordedDev/sudoku:main
merge into: retoor:main
Conversation 3 Commits 19 Files Changed 8 +6 -3
1 changed files with 6 additions and 3 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit ec3b55b2f3 - Show all commits

7
reviews/BordedDev/notes.md
View File

@ -2,13 +2,13 @@ It doesn’t render correctly on firefox but does in edge, it’s a rectangle, n
Since the scaling is based on the font size, using `em` + `line-height: 1` allows it to scale correctly by adding it to Since the scaling is based on the font size, using `em` + `line-height: 1` allows it to scale correctly by adding it to
`.sudoku-field`. `.sudoku-field`.
Fixing it by wrapping the numbers and adding `height: 100%` fixes the individual cells, but the whole table is still a Fixing it by wrapping the numbers and adding `height: 100%` fixes the individual cells, but the whole table is still a
rectangle but causes the cells to overlap. rectangle but causes the cells to overlap (inside out size issues was excuse).
There is a bunch of unused code There is a bunch of unused code
Autosolver is broken, I had to move the function around Autosolver is broken, I had to move the function around
retoor commented 2025-01-16 18:37:59 +00:00
Outdated
Review
Copy Link

Ah, sad. I didn't know status of the code. But if it's working again, i will put the sudoku on my site.

Ah, sad. I didn't know status of the code. But if it's working again, i will put the sudoku on my site.
Switched .forEach to for loop for performance (recommended practice) Switched .forEach to for loop for performance (recommended practice, but honestly it’s not a big deal)
Convert the keyboard key lookup to a map, mostly stylistic but I imagine it’s also easier to edit. Convert the keyboard key lookup to a map, mostly stylistic but I imagine it’s also easier to edit.
@ -25,3 +25,6 @@ It's not recommended to setup the elements in the constructor https://developer.
You can use `||=` for assigning to an undefined variable (if it's undefined) You can use `||=` for assigning to an undefined variable (if it's undefined)
Borders are on all grid elements, causing a double border on the edges Borders are on all grid elements, causing a double border on the edges
I'd recommend running Biome.js or ESlint + Prettier to clean up the code
(if you're using ML you can even make these strict enough about ordering and naming conventions)