Roman Numeral Converter/My first test project

Code Feedback
1

This was my attempt at learning just the very basics of using HTML and JS together while also trying to push my very, very, did I say very?, limited JS knowledge thus far. I would greatly appreciate any feed back on what I could improve upon such as “hey you don’t need to use 10,000 curly brackets”, or anything I’m doing that should be stopped now before it becomes a bad habit, or even just a note of how I could make my current sample project more efficient. Here’s the link to my codepen: Roman Numeral Converter
Edit: I played around with the CSS quite a bit (it felt like I had forgotten everything from prior lessons) and would love any feedback on that part as well. I struggled a whole bunch with making things line up and I feel as though there are much better ways to do it then what I did.

2

I see you focused more on the JS rather than the html and css part. I don’t know any JS so I can’t comment on your code, but the app seems to work properly, so, great job!

You could style it a bit with css to make it look cooler though.

1 Like
3

I 100% agree, it’s pretty boring right now, and I’m actually working on it right now to make it nicer looking. Thanks for the feed back.

4

Hi @yeahman,

HTML

  • Spaces make attributes and values hard to read:
<input id = "num" placeholder = "number or roman numeral">
<div id = "display">
</div>
<button id = "click">Click for results</button>

JavaScript

  • Lack of consistency:
    event listeners and on-event handlers in the same script
...

  inputBox.addEventListener("keyup",function(key){
...

  document.getElementById("click").onclick = 
...

Cheers and happy coding :slight_smile:

1 Like
5

Thanks for pointing these out as I had no idea, I’m sure it was obvious :grin:. I’ve made the adjustments to my HTML and will try to remember that going forward. This was my introduction to using event listeners and such and after reading your feedback I’ve done a bunch more reading up on them. I’ve adjusted my JS code and removed the inline event handler for an event listener instead. Thanks again!

1 Like
7

Thanks, I admit I should have noticed these before hand. I’ve since made the corrections. I really appreciate the feedback!