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.

Sign up for GitHub

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

Jump to bottom

Redirect fixed #2494

Merged
merged 3 commits into from
Nov 19, 2021
Merged

Redirect fixed #2494

merged 3 commits into from
Nov 19, 2021

Conversation

sirinoks
Copy link
Contributor

Issue This PR Addresses

Fixes #2454

Type of Change

  • Bugfix: Now redirects to the required path

Description

Fixes issue where the root path was displaying wrong. Also fixes one where root is 404
No screenshots, since before the pages didn't work, and now they do.

Checklist

  • Quality: This PR builds and passes our npm test and works locally
  • Tests: This PR includes thorough tests or an explanation of why it does not
  • Screenshots: This PR includes screenshots or GIFs of the changes made or an explanation of why it does not (if applicable)
  • Documentation: This PR includes updated/added documentation to user exposed functionality or configuration variables are added/changed or an explanation of why it does not(if applicable)

@gitpod-io
Copy link

gitpod-io bot commented Nov 19, 2021

@sirinoks sirinoks linked an issue Nov 19, 2021 that may be closed by this pull request
Npm run dev not working correctly in status api folder #2454
Closed
@humphd
Copy link
Contributor

humphd commented Nov 19, 2021

I'm confused about this vs https://github.com/Seneca-CDOT/telescope/pull/2493/files, which merged last night cc @drew5494.

@sirinoks sirinoks requested a review from drew5494 November 19, 2021 15:05
@sirinoks
Copy link
Contributor Author

I'm confused about this vs https://github.com/Seneca-CDOT/telescope/pull/2493/files, which merged last night cc @drew5494.

Yea, I noticed they did a similar thing. It didn't pull their changes for some reason when I was making mine, only showed them when I made a PR. I'm pretty sure it still broke because of the host variable. Shouldn't conflict at this point.

@humphd
Copy link
Contributor

humphd commented Nov 19, 2021

If you want to build on that code, please rebase so we isolate your new changes.

@sirinoks
Copy link
Contributor Author

These lines were replaced by a more compact single line

This is basically redundant, since that's what those lines did

This fixes a different issue of 404 page on '/'.

This + this fixes an issue when the build fails if you forgot to put an env. variable in env.local.

humphd
humphd requested changes Nov 19, 2021
View reviewed changes
Copy link
Contributor

@humphd humphd left a comment

Choose a reason for hiding this comment

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

OK, your explanation helps me understand. Thanks for this.

src/api/status/src/server.js Show resolved Hide resolved
src/api/status/src/server.js Outdated Show resolved Hide resolved
src/api/status/src/server.js Outdated Show resolved Hide resolved
@sirinoks
Improved based on review
3028a01 
Localhost is now consistent in API_HOST
API_HOST is now in env.local
Merged redirect logic
Removed useless comment
@sirinoks
Copy link
Contributor Author

Code changed based on review

humphd
humphd approved these changes Nov 19, 2021
View reviewed changes
Copy link
Contributor

@humphd humphd left a comment

Choose a reason for hiding this comment

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

Code looks right, let's get a couple of people to test this locally.

Andrewnt219
Andrewnt219 approved these changes Nov 19, 2021
View reviewed changes
Copy link
Contributor

@Andrewnt219 Andrewnt219 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Great catch, it feels right to just run yarn dev and everything is working. No errors on my local.

One unrelated change I don't know if you can sneak it in this PR. Should we change the default server port to 1112? 1111 is used by npm run services:start posts.

@humphd
Copy link
Contributor

humphd commented Nov 19, 2021

One unrelated change I don't know if you can sneak it in this PR. Should we change the default server port to 1112? 1111 is used by npm run services:start post.

Why would you want to do this? Posts uses 5555 https://github.com/Seneca-CDOT/telescope/blob/master/config/env.production#L118. Can you say more about what you are trying to do?

@Andrewnt219
Copy link
Contributor

Andrewnt219 commented Nov 19, 2021 •
edited
Loading

@humphd Oh wait, my bad, I was running npm run services:start instead of npm run services:start posts.

@humphd humphd merged commit ec5ddd2 into Seneca-CDOT:master Nov 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@humphd humphd humphd approved these changes

@Andrewnt219 Andrewnt219 Andrewnt219 approved these changes

@drew5494 drew5494 Awaiting requested review from drew5494

@menghif menghif Awaiting requested review from menghif

@suhhee1011 suhhee1011 Awaiting requested review from suhhee1011

@tuanthanh2067 tuanthanh2067 Awaiting requested review from tuanthanh2067

@AmasiaNalbandian AmasiaNalbandian Awaiting requested review from AmasiaNalbandian

@hphan9 hphan9 Awaiting requested review from hphan9

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Npm run dev not working correctly in status api folder
3 participants
@sirinoks @humphd @Andrewnt219