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

Remove home layout #1550

Merged
merged 5 commits into from
Jun 15, 2022
Merged

Remove home layout #1550

merged 5 commits into from
Jun 15, 2022

Conversation

damianhxy
Copy link
Member

@damianhxy damianhxy commented Jun 9, 2022

Description

  • Delete home layout
  • Make course#index use application layout instead
  • Make devise use application layout instead

Screen Shot 2022-06-12 at 10 29 10

- Remove dead code from application layout

Motivation and Context

Currently, the home layout is almost a duplicate of the application layout, and is only used when rendering the home page (i.e. courses#index). In fact, the only difference when rendering with the application layout is that the tab title is "Autolab" instead of "Autolab Home".

In order to avoid the need to duplicate future code changes across both layouts, this PR deletes the home layout and makes courses#index default to the usual application layout.

How Has This Been Tested?

  • Used RubyMine grep to ensure no other route renders with the home layout

    • For reference, here are all the usages of layout: Screen Shot 2022-06-09 at 01 01 51
  • Diffed home layout with new application layout and ensured that any differences are inconsequential (notably, it has additional includes, roster_error flash handling, and persistent announcements)

  • Ensured that home page looks the same and functions the same (but no surprises there)

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • I have run rubocop for style check. If you haven't, run overcommit --install && overcommit --sign to use pre-commit hook for linting
  • My change requires a change to the documentation, which is located at Autolab Docs
  • I have updated the documentation accordingly, included in this PR

@20wildmanj
Copy link
Contributor

When logged out, when I try to go to home (e.g. localhost:3000) I get a 404 not found error. I couldn't find a simple fix as well (I thought that since you removed render layout: "home" in app/controllers/courses_controller.rb I tried replacing that line with render layout: "application" but that didn't fix it). I also get a 404 error just by going to http://localhost:3000/auth/users/sign_in but not for developer log in. (Actually it seems like the redirect to the sign-in causes a 404 as well)

Screen Shot 2022-06-10 at 17 37 32

Copy link
Contributor

@20wildmanj 20wildmanj left a comment

Choose a reason for hiding this comment

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

Home page works now for not logged in users, I found a couple more references to "home" in home_controller.rb (line 36, 52) but they seem to be unused so it shouldn't affect anything.

LGTM!

@damianhxy
Copy link
Member Author

Home page works now for not logged in users, I found a couple more references to "home" in home_controller.rb (line 36, 52) but they seem to be unused so it shouldn't affect anything.

Note: The references are to home#index, i.e. the index action of the home controller, not the home layout. That said, it seems that the action is missing from the controller.

@damianhxy damianhxy merged commit f6b726d into master Jun 15, 2022
@damianhxy damianhxy deleted the remove-home-layout branch June 15, 2022 16:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants