Skip to content

Conversation

@ghost
Copy link

@ghost ghost commented Jun 20, 2019

Here are my edits for the groups-sorter app, along with its other dependencies such as the extra icons and added css and javascript. I created a new UI design for the sorter application. So, now, the user can click on a subject and it'll bring the time groups up on separate columns. One column for Full Time, and the other for Part Time. Please, take a look at the differences and review. Also, feel free to add or take what you may to make this a better version. :)

edit this

  • I have read freeCodeCamp's contribution guidelines.
  • My pull request has a descriptive title (not a vague title like Update index.md)
  • My pull request targets the master branch of freeCodeCamp.
  • None of my changes are plagiarized from another source without proper attribution.
  • All the files I changed are in the same world language (for example: only English changes, or only Chinese changes, etc.)
  • My changes do not use shortened URLs or affiliate links.

Closes #XXXXX

Here are my edits for the groups-sorter app, along with its other dependencies such as the extra icons and added css and javascript. I created a new UI design for the sorter application. So, now, the user can click on a subject and it'll bring the time groups up on separate columns. One column for Full Time, and the other for Part Time. Please, take a look at the differences and review. Also, feel free to add or take what you may to make this a better version. :)
@ghost ghost requested review from JonDevOps and hannahpi June 20, 2019 23:32
Copy link
Member

@JonDevOps JonDevOps left a comment

Choose a reason for hiding this comment

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

Reviewing changes now.

Copy link
Member

@JonDevOps JonDevOps left a comment

Choose a reason for hiding this comment

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

Everything looks great I am approving this commit.

@JonDevOps JonDevOps merged commit 68ed975 into master Jun 22, 2019
@JonDevOps JonDevOps deleted the LeeTheTweak-sorter-app-1 branch June 23, 2019 05:27
Copy link
Member

@hannahpi hannahpi left a comment

Choose a reason for hiding this comment

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

I know I'm late as this has already been merged. Overall it looks good. I'm not too keen on reviewing css from looking at code but everything else looks good. Hopefully we can identify why github picked up changes on all lines of these files and prevent that in the future. You might want to check settings and see if there's a setting to conserve current line endings in your editor.

Thanks for your contribution!

margin-bottom: 50px;
font-family: Montserrat, "Helvetica Neue", Helvetica, Arial, sans-serif;
}
}
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure why github is picking up diffs on all these files when no changes have been made. I'm wondering if you may have inadvertently switched line endings, that's the only invisible character I would expect here that would cause a change on all these files. If it's necessary great, In the future I would prefer it to be in a different PR so it's easier to sort out the changes.

}

</script>
<!-- 6/9/2019 Chris L: Created an external js file in the js folder, Script to sort study groups-->
Copy link
Member

Choose a reason for hiding this comment

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

This comment probably belongs in the pull request rather than in the file or the commit messages to make it easier to review what was done. The note that it's a script for sorting study groups is fine though so either way.

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