up-for-grabs.net icon indicating copy to clipboard operation
up-for-grabs.net copied to clipboard

Published 20 hours ago • verified publisher icon up-for-grabs

Code tidiness

Open ghost opened this issue 3 years ago • 9 comments
trafficstars

I've been looking through the code, and it seems pretty difficult to work on.

The code isn't written very well, it's hard to read, and doesn't follow many best practices.

Also, I had to do a lot of guessing to figure out how to test my changes. There isn't a clear guide to it.

Examples

Bad best practices

Front-end

~~Loading an excessive amount of inline JS in the <head>.~~

Nevermind! I thought the scripts.html was 1.) larger and 2.) being loaded in the head. It's fine.

Back-end

The current version of Ruby is somewhat outdated.

Messy code

Poor spacing

image (This had many other spacing issues but I have changed them)

Bad naming

The naming of the folder javascripts is not correct in English. image

It is incredibly difficult to understand where the JavaScript code is from the names. This also relates to the scripts.html issue.

Misc.

This should be included in head.html, not in index.html. image

ghost avatar Dec 19 '21 22:12 ghost

@ConnorAhern thanks for starting this discussion:

Also, I had to do a lot of guessing to figure out how to test my changes. There isn't a clear guide to it.

There's a section in the CONTRIBUTING.md about testing that might be of use, but that likely needs an update too.

Some other quick thoughts while I'm here:

Loading an excessive amount of inline JS in the <head>.

Not sure I understand this one - got an example handy to share?

The current version of Ruby is somewhat outdated.

Yeah, I'm not aware of any blockers on using Ruby 3 but the 2.6 that we use in the container isn't too far behind:

https://github.com/up-for-grabs/up-for-grabs.net/blob/195149ca60349a917219b411a641058b2cf6aae3/Dockerfile#L1

Poor spacing

I'd love for this script - which we added in #2885 - to become a standalone script which would mean we can lint it like our other scripts...

The naming of the folder javascripts is not correct in English.

This is fair, likely just something we did years ago when setting this up that we've lived with since.

This should be included in head.html, not in index.html.

Not sure I agree with this one, as this is content that will render when JavaScript is disabled in the browser, so it should be within <body> so that it can be styled like other DOM elements.

Happy to chat more about history here, and if you're feeling like trying to improve the code you can open draft PRs which should create Netlify test URLs to validate the work (which simplifies reviews on our end).

shiftkey avatar Dec 20 '21 13:12 shiftkey

Not sure I agree with this one, as this is content that will render when JavaScript is disabled in the browser, so it should be within <body> so that it can be styled like other DOM elements

Ah, I see. My bad.

There's a section in the CONTRIBUTING.md about testing that might be of use, but that likely needs an update too.

Oops - didn't notice that! Thanks.

ghost avatar Dec 20 '21 17:12 ghost

Not sure I understand this one - got an example handy to share?

I'll edit it into the post in a second.

Edit: Nevermind! It's fine.

ghost avatar Dec 20 '21 17:12 ghost

I'd love for this script - which we added in #2885 - to become a standalone script which would mean we can lint it like our other scripts...

Do you mean link or lint? If you mean link, yes, I agree. If you mean lint, can you clarify?

ghost avatar Dec 20 '21 17:12 ghost

Hey, I was trying to follow the instructions in CONTRIBUTING.md. The first thing I did when trying to "guess" how to test it was using the bundle commands, but I got this error:

  Please add the following to your Gemfile to avoid polling for changes:
    gem 'wdm', '>= 0.1.0' if Gem.win_platform?
 Auto-regeneration: enabled for 'C:/Users/bekfe/OneDrive/Documents/Projects/up-for-grabs.net/up-for-grabs.net'
C:/Users/bekfe/OneDrive/Documents/Projects/up-for-grabs.net/up-for-grabs.net/vendor/bundle/ruby/3.0.0/gems/jekyll-3.9.0/lib/jekyll/commands/serve/servlet.rb:3:in `require': cannot load such file -- webrick (LoadError)
        from C:/Users/bekfe/OneDrive/Documents/Projects/up-for-grabs.net/up-for-grabs.net/vendor/bundle/ruby/3.0.0/gems/jekyll-3.9.0/lib/jekyll/commands/serve/servlet.rb:3:in `<top (required)>'
        from C:/Users/bekfe/OneDrive/Documents/Projects/up-for-grabs.net/up-for-grabs.net/vendor/bundle/ruby/3.0.0/gems/jekyll-3.9.0/lib/jekyll/commands/serve.rb:184:in `require_relative'
        from C:/Users/bekfe/OneDrive/Documents/Projects/up-for-grabs.net/up-for-grabs.net/vendor/bundle/ruby/3.0.0/gems/jekyll-3.9.0/lib/jekyll/commands/serve.rb:184:in `setup'
        from C:/Users/bekfe/OneDrive/Documents/Projects/up-for-grabs.net/up-for-grabs.net/vendor/bundle/ruby/3.0.0/gems/jekyll-3.9.0/lib/jekyll/commands/serve.rb:102:in `process'
        from C:/Users/bekfe/OneDrive/Documents/Projects/up-for-grabs.net/up-for-grabs.net/vendor/bundle/ruby/3.0.0/gems/jekyll-3.9.0/lib/jekyll/commands/serve.rb:93:in `block in start'
        from C:/Users/bekfe/OneDrive/Documents/Projects/up-for-grabs.net/up-for-grabs.net/vendor/bundle/ruby/3.0.0/gems/jekyll-3.9.0/lib/jekyll/commands/serve.rb:93:in `each'
        from C:/Users/bekfe/OneDrive/Documents/Projects/up-for-grabs.net/up-for-grabs.net/vendor/bundle/ruby/3.0.0/gems/jekyll-3.9.0/lib/jekyll/commands/serve.rb:93:in `start'
        from C:/Users/bekfe/OneDrive/Documents/Projects/up-for-grabs.net/up-for-grabs.net/vendor/bundle/ruby/3.0.0/gems/jekyll-3.9.0/lib/jekyll/commands/serve.rb:75:in `block (2 levels) in init_with_program'
        from C:/Users/bekfe/OneDrive/Documents/Projects/up-for-grabs.net/up-for-grabs.net/vendor/bundle/ruby/3.0.0/gems/mercenary-0.3.6/lib/mercenary/command.rb:220:in `block in execute'
        from C:/Users/bekfe/OneDrive/Documents/Projects/up-for-grabs.net/up-for-grabs.net/vendor/bundle/ruby/3.0.0/gems/mercenary-0.3.6/lib/mercenary/command.rb:220:in `each'
        from C:/Users/bekfe/OneDrive/Documents/Projects/up-for-grabs.net/up-for-grabs.net/vendor/bundle/ruby/3.0.0/gems/mercenary-0.3.6/lib/mercenary/command.rb:220:in `execute'
        from C:/Users/bekfe/OneDrive/Documents/Projects/up-for-grabs.net/up-for-grabs.net/vendor/bundle/ruby/3.0.0/gems/mercenary-0.3.6/lib/mercenary/program.rb:42:in `go'
        from C:/Users/bekfe/OneDrive/Documents/Projects/up-for-grabs.net/up-for-grabs.net/vendor/bundle/ruby/3.0.0/gems/mercenary-0.3.6/lib/mercenary.rb:19:in `program'
        from C:/Users/bekfe/OneDrive/Documents/Projects/up-for-grabs.net/up-for-grabs.net/vendor/bundle/ruby/3.0.0/gems/jekyll-3.9.0/exe/jekyll:15:in `<top (required)>'
        from C:/Users/bekfe/OneDrive/Documents/Projects/up-for-grabs.net/up-for-grabs.net/vendor/bundle/ruby/3.0.0/bin/jekyll:23:in `load'       
        from C:/Users/bekfe/OneDrive/Documents/Projects/up-for-grabs.net/up-for-grabs.net/vendor/bundle/ruby/3.0.0/bin/jekyll:23:in `<main>'

I only recently started coding in Jekyll/Ruby, so I'm not sure what to make of these errors. Is there something I typed wrong on my end?

BTW, bundle install works fine, but it's bundle exec jekyll serve that is not working for me.

ghost avatar Dec 20 '21 17:12 ghost

Do you mean link or lint? If you mean link, yes, I agree. If you mean lint, can you clarify?

Not a typo - "lint" in this context means formatting our scripts in a consistent pattern. We use ESLint with a set of rules defined here:

https://github.com/up-for-grabs/up-for-grabs.net/blob/cd14abae7bac9629b05a47f60431f7999cd12f24/.eslintrc.json#L1-L18

This means we can just run npm run lint and catch any errors, or npm run lint-fix to try and auto-fix anything:

https://github.com/up-for-grabs/up-for-grabs.net/blob/cd14abae7bac9629b05a47f60431f7999cd12f24/package.json#L11-L12

BTW, bundle install works fine, but it's bundle exec jekyll serve that is not working for me.

Oooh, Windows. Thanks for that clue, and it's been a while since I've tried to do this on Windows so perhaps something's changed there.

shiftkey avatar Dec 20 '21 17:12 shiftkey

Ah, I see what you mean. I haven't used ESLint before.

ghost avatar Dec 20 '21 17:12 ghost

Apologies for the delay here, but I'm only just now finding some spare time for this after working through some automation headaches yesterday. I've merged #3025 to ensure that Windows contributors can work with the solution, but let me know if you spot anything else unexpected @ConnorAhern.

shiftkey avatar Feb 13 '22 17:02 shiftkey

Seeing the code I guess it is written in Visual Studio Code. If that's the case, just use the format option in visual studio code. You will get it by right clicking in the empty sapce besides the code. Else, copy the code to visual studio code and then format it.

intelkid7 avatar Jul 12 '22 10:07 intelkid7

Closing this out as the original author has deleted their account.

Please open a fresh issue if there are things to discuss.

shiftkey avatar Sep 19 '22 14:09 shiftkey