Closed Add00 closed 3 years ago
Great idea :) This is overall looking good, I have multiple nitpickings so that the extension reach an almost perfect state:
A few feedbacks:
ColorConversion
hexadecimal
. Add color
. Don't use plural: conversion
Convert RGB color values into various other colour formats and vice versa.
name to RGB
, either use an explicit object containing the values (const colorNameToRgb = { red: "255;0;0", ... }
), or remove the function. We don't use as much as possible CSS/getComputedStyle/getPropertyValue. It's implementation specific and it forces to create a div, which will create memory pressure on the garbage collector and is super inefficient. Basically never ever use DOM manipulation for games.minRGB == maxRGB
=> minRGB === maxRGB
. Never use ==
.let H = 0
=> let h = 0
(use camelCase for variables, not PascalCase).gdjs.rgbToHex
;) See https://docs.gdevelop-app.com/GDJS%20Runtime%20Documentation/gdjs.html#.rgbToHexconst n = eventsFunctionContext.getArgument("n"); let hex = n;
. Just name your parameter "hex".Finally, for all functions can you verify what is going on if you enter an invalid value as a parameter? Thanks! :)
Thanks for taking the time to review my extension. I think I've added all your feedback so let me know what you think.
NameToRgb
function, so I reverted the code back to that version to be more efficient.RgbToHex
with the built in method.Thanks! Looks great :) I've fixed a few grammar mistakes ("an" => "a"), added periods, and changed the error handling so that all errors are logged in the console, and a default value is returned (black in all cases, i.e: "0;0;0").
Right now I have place holder text there since I'm not sure what the message content should be.
Usually, I choose something like console.error("Unable to do blabla:", error)
with error
being the exception or the message describing what went wrong.
Thanks for working on this! :)
Describe the extension
A simple extension that converts rgb values into other colour formats.
Checklist
Extension file
colourConversions.zip