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

/search/:username + test update #7

Merged
merged 6 commits into from Oct 19, 2019
Merged

/search/:username + test update #7

merged 6 commits into from Oct 19, 2019

Conversation

theRJMurray
Copy link
Owner

Added:
-added /search/:username to differentiate between a search and the default root
-reverted npm test to previous form, added /search/:username to pass tests

Working off multiple pull requests not committed to master. Wondering if there will be any problems that pop up because of that.

Realizing that I am going to have to connect this backend to a frontend to display the data how I want it. Deciding between raw html or React.js.

Going to Process JSON Data from project board before I decide.

Forgot to update the readme.

res.sendFile(path.join(__dirname + 'index.html'))
});

app.get('/search/:username', function(req, res, error){
Copy link
Collaborator

Choose a reason for hiding this comment

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

You might want to update the readme to reflect this

Copy link
Collaborator

Choose a reason for hiding this comment

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

Does the stuff in index.html show up when you hit this route?

Copy link
Owner Author

Choose a reason for hiding this comment

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

yes it does

Copy link
Collaborator

Choose a reason for hiding this comment

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

Then I understand less than I thought I did

Copy link
Owner Author

Choose a reason for hiding this comment

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

No sorry I was mistaken. When you hit the :username route, the index.html does NOT show up. It only shows up when you hit the '/' root.

<h1>Welcome to RuneHelper!</h1>
<p>To check your stats, change the url:</p>
<p>Example:</p>
<p>localhost:3000/INSERT_USERNAME_HERE</p>
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should also be updated to include search

@turtleman270
Copy link
Collaborator

It might have made things easier/ more clear if you only used one branch/PR

You could have committed and pushed additional changes to the original branch instead of creating multiple PRs

`npm start`
`npm start`

## Searching User Stats
In your browser go to localhost:3000/username where username is the
Copy link
Collaborator

Choose a reason for hiding this comment

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

3000/search/username

@theRJMurray
Copy link
Owner Author

Oh I see now. I was thinking that pull requests were immutable for some reason.

@turtleman270
Copy link
Collaborator

If you merge this, it'll have all the changes in #5 and #6

@theRJMurray
Copy link
Owner Author

so #5 and #6 can be cancelled, and this one merged?

@turtleman270
Copy link
Collaborator

so #5 and #6 can be cancelled, and this one merged?

I think they might be automatically cancelled because all the commits in those PR's exist in this PR

@theRJMurray theRJMurray merged commit 9f22c67 into master Oct 19, 2019
@theRJMurray
Copy link
Owner Author

Many lessons learned on that simple pr.

I'll be sure to make a more complete pr next time.

Thanks for the review!

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.

None yet

2 participants