Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Redesign and modernize lockup #56

Closed
wants to merge 28 commits into from
Closed

Redesign and modernize lockup #56

wants to merge 28 commits into from

Conversation

dankimio
Copy link
Contributor

@dankimio dankimio commented Aug 16, 2019

This is a very big pull request. I would love to get some feedback on it first, and then I can split it into smaller chunks, revert unwanted changes if there are any. Then we can merge them one-by-one.

This pull request should resolve issues #54, #55.

Major changes

  • Removed upper version limit for Rails. Previously it was limited to versions below 6.0.0. Not sure if we need to restrict major versions, because we don't have much Rails-specific code that can be broken in any future versions of Rails. It should work with Rails 6 now (released today).
  • Cleaned up and updated Rakefile. Much of the functionality is now included in Bundler. No need for so many lines of code in Rakefile now.
  • Use system fonts, using Tailwind's sans-serif.
  • I went with a white background, which looks more neutral. If we do things right, we should adopt dark-mode APIs (e.g. prefers-color-scheme: dark in Safari)
  • Removed old media queries. Implemented mobile-first responsive design using flexbox (98.6% browser support as of August 2019) with only 1 media query for desktop screens.
  • Updated color scheme, I went with softer colors from Tailwind CSS's color palette.
  • Code word field is now required.
  • Added mini-normalize. Might be helpful for consistency across different browsers and future changes.
  • Text updates. I split the heading into to parts: h1 with One more step (inspired by CloudFlare's robot pages) and description paragraph, which should be more semantic.
  • Implemented hints without JS. I went with title HTML attribute and help cursor.
  • Updated design for alerts.

Minor changes

  • Removed ampersand based characters (…) since we're using UTF-8.
  • Cleaned up and updated code style. The project doesn't have a Rubocop configuration or any style guides. Some parts were written using single quotes and some using double quotes. I converted all occurrences to single quotes, but this can be changed to double quotes easily if needed.
  • Upgrade local Ruby version to 2.6.3

Things to consider

  • I'm thinking of getting rid of inline_css and normalize partials and inlining everything in the layout.
  • I haven't tested this with Edge, but should be working fine since we're not using anything except Flexbox.

Screenshots

This is how it looks like on Desktop screens

Screenshot 2019-08-16 at 23 25 30

@dankimio dankimio marked this pull request as ready for review August 16, 2019 20:47
@gblakeman
Copy link
Member

@dankimio Some killer work here, thank you! This project started long before I was aware of things like rubocop or setting good style guides, so it’ll be nice to tighten it up and modernize it.

I’m going to give the changes some more thorough review, testing, and thought as soon as I can.

@gblakeman
Copy link
Member

@dankimio I’ve spent a bit of time with this…

Overall, I’m a fan of updating the architecture, the design, etc. The philosophical issue I’m running into is backwards compatibility. I’m open to input from others, but as it stands, a lot of the changes that take advantage of more modern Ruby/Rails syntax break the gem for Rails 3.X and in some cases Rails 4.X.

Ideally, we could incorporate a lot of this work you’ve done, but without breaking the gem for older versions. At its core, it’s a really simple gem, so I’m having trouble trying to convince myself that it’s worth ditching older Rails versions.

What do you think?

@danrabinowitz do you have any thoughts along these lines?

@dankimio
Copy link
Contributor Author

dankimio commented Sep 7, 2019

Worth noting that Rails versions below 5.1 and Ruby versions below 2.4 are no longer maintained, they do not receive security updates and their use is discouraged. For older apps there's always an option to use older versions of the gem.

We could revert safe navigation operator, this would be an easy fix.

Copy link
Collaborator

@danrabinowitz danrabinowitz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, I really like this. I suggest splitting it into smaller PRs. I'd aim for:

  1. The pure refactor changes (from rubocop, I assume) such as replacing double quotes with single quotes, etc. Ideally, if you're going to use rubocop, you'd include the rubocop gem as a development dependency and include a rubocop.yml
    This PR would have little risk. It's just about style.

  2. The css, HTML, JS changes. This PR would have low risk related to ruby versions, but we'd just need to check against older browsers. (Maybe not a huge issue these days?)

  3. The ruby changes, such:

user_agent && user_agent.downcase.match(CRAWLER_REGEX)

vs

user_agent&.downcase&.match(CRAWLER_REGEX)

That particular change is NOT a good idea because of older ruby versions, but there might be other ruby changes buried in this PR that we could consider.

Thanks for the contribution! I think most of these changes would be GREAT to include.

@@ -11,7 +11,7 @@ class LockupController < Lockup::ApplicationController
def unlock
if params[:lockup_codeword].present?
user_agent = request.env['HTTP_USER_AGENT'].presence
if user_agent && user_agent.downcase.match(CRAWLER_REGEX)
if user_agent&.downcase&.match(CRAWLER_REGEX)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not a good idea, as it will break for older ruby versions.

@gblakeman
Copy link
Member

@danrabinowitz Thanks for the input Dan. I like the idea of separate PR’s as well. I can get a CI-solution running on this repo too so that we can automate tests and rubocop.

@dankimio dankimio mentioned this pull request Nov 3, 2019
@dankimio dankimio closed this Nov 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants