search

danfstucky / LeagueSpec

Rails application for League of Legends statistics
1 stars 0 forks source link

First page Demo #5

Closed sulaimonlasisi closed 8 years ago

sulaimonlasisi commented 8 years ago

I designed a page that can be accessed via localhost/profiles Let me know what you think. Most of the changes made were in app and config folders. @jwiebe17 @danfstucky

danfstucky commented 8 years ago

Is profiles going to be a page that displays a user's LoL profile stats?

sulaimonlasisi commented 8 years ago

I made all the changes Dan highlighted in the new version of the branch. Make sure to look at it and leave a comment.

danfstucky commented 8 years ago

main and container are both vague class names. How about main_profile and something more descriptive for container as well. What is it a container for? We will eventually have a lot of html classes and this will help keep things straight.

sulaimonlasisi commented 8 years ago

I pushed indentation and naming convention changes to the first_page branch

danfstucky commented 8 years ago

+1 After Joel give his approval, make sure you merge to master using git merge --squash so the entire pull request will be viewed as a single commit into master. This makes documenting and tracking things down a lot easier and cleaner in the future.

jwiebe17 commented 8 years ago

+1

danfstucky commented 8 years ago

When are you going to merge this @sl7v3 ? I know the profiles page will be changing a lot, but this should still be merged to master and changed in a later issue. When you merge to master, make sure you first pull down the most recent master and merge that into your local branch. Then merge your local branch into your local master and push master. This is because there have been changes to master since this branch was created. You will get merge conflicts if you don't do it this way.

sulaimonlasisi commented 8 years ago

Merged this and will close the pull request.

danfstucky commented 8 years ago

@sl7v3 This was not properly merged to master. Your merge commit does not contain any of the changes that were made during this pull request. Do you still have your local copy of this branch?

sulaimonlasisi commented 8 years ago

@danfstucky I assume that you expected some functionality that I did not include in the most recent merge. I only implemented the indentation and naming convention you talked about. However, I will reopen the pull request and merge the current status of the profile page. It should also comply with indentation and naming conventions from the last time.

@danfstucky I pushed my master to origin/master. I hope that is the version you were expecting. If not, I can revert if needed.

danfstucky commented 8 years ago

No, the functionality you had for this branch was fine. For example, if I go to master right now and look at the profiles stylesheet, there is no nesting of styles. One of the changes you made in this pull request was in response to a comment about nesting styles, such as making .main {} and .main h1 {} into simply .main { h1 {} }. When you merge a branch into master and push the updated master, it should contain the final form of the branch changes after the code review. That way, only reviewed code is ever incorporated into master. The code you incorporated into master doesn't contain some of the changes we discussed in this review.

sulaimonlasisi commented 8 years ago

@danfstucky I pushed the changes up now. I really have no explanation for why it did not appear on origin/master before. Let me know if it conforms with the changes you expect.

danfstucky commented 8 years ago

@sl7v3 At this point, it looks like you have completely changed the profiles stylesheet from what was in this pull request. That's fine, since this pr was mainly just practice anyway, but make sure you are pushing new code to master only after it has passed a code review.

sulaimonlasisi commented 8 years ago

@danfstucky I agree with your observation. I will keep that in mind in the future.