Conversation
|
I reopened this PR as its using the branch based off dev. The other PR had additional changes not related to the about us page. |
ddigioia3
left a comment
ddigioia3
left a comment
There was a problem hiding this comment.
Bolting about us onto sessions is probably a bad pattern to follow. Lets seperate those out using the methodology explained in the treehouse blog I linked. 👍
| } | ||
|
|
||
| .footer { | ||
| bottom: 0; |
There was a problem hiding this comment.
[Code Hygiene]
If this is not longer needed, its cleaner to just remove it over commenting it out.
| @@ -0,0 +1,14 @@ | |||
| <div class "container"> | |||
There was a problem hiding this comment.
[Separation of Responsibilities]
The sessions are responsible for handling authentication/authorization, not really about handling static pages. Putting the about page here creates what's known as "tight coupling" between static pages and sessions, which is bad because it makes maintenance of the two difficult because if you change one the other may get broken.
Instead, we should add separate handling for static pages.
This treehouse blog explains how to accomplish this.
| get 'login', to: 'sessions#new' | ||
| post 'login', to: 'sessions#create' | ||
| get 'welcome', to: 'sessions#welcome' | ||
| get 'about', to: 'sessions#about' |
There was a problem hiding this comment.
Once you decouple the sessions from static pages, this will need to change as well
|
Hi @ddigioia3 ... I no longer have this branch as I would have realized I made a mess of things. I re-cloned the repository and created new branches. Can I create another branch on my new repository with the suggested changes and do a new pull request? |
|
@aligiselle were you able to checkout this branch and continue your work? |
|
Hi @myang1010 when I did that it caused serious errors. It seemed to delete everything I pulled on dev and tried to add a couple of files from this branch but curiously never switched to it. |
3 participants
Description
(What does the PR do? What's problem? What's the solution?)
(If this is a bugfix, what is the issue? How/why are things breaking currently?)
Background Information
(Any background information/context for the changes in the PR?)
(Is there a deeper root cause not addressed by this PR?)
Changelog
Internal changes
External changes
Implications for
devbranch(Is there anything the rest of the team should be aware of after merging this branch? (ie. migrations/scripts to run, libraries to install, etc.))
Screenshots/Recordings
(Please include before/after screenshots of any user-facing changes)
PR Checklist
console.log,byebug,puts,prints,p,ppHow does this PR make you feel?
Select a gif or emoji and add here. You can find gifs here: https://giphy.com/categories/emotions
