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

Move frontend from src/web to src/web/app #3287

Merged
merged 1 commit into from
Mar 24, 2022
Merged

Move frontend from src/web to src/web/app #3287

merged 1 commit into from
Mar 24, 2022

Conversation

menghif
Copy link
Contributor

@menghif menghif commented Mar 22, 2022

Issue This PR Addresses

Partial work for #2993

I'm breaking this issue into multiple PRs to make reviews easier.

Type of Change

  • Bugfix: Change which fixes an issue
  • New Feature: Change which adds functionality
  • Documentation Update: Change which improves documentation
  • UI: Change which improves UI

Description

This PR moves our frontend web app from src/web to src/web/app.

Steps to test the PR

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)

@vercel
Copy link

vercel bot commented Mar 22, 2022 •
edited
Loading

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/humphd/telescope/523JNxEge5jwNXiYXXNnsiYW9qGx
✅ Preview: Canceled

[Deployment for 58d317a canceled]

@gitpod-io
Copy link

gitpod-io bot commented Mar 22, 2022

cindyorangis
cindyorangis previously approved these changes Mar 22, 2022
View reviewed changes
DukeManh
DukeManh previously approved these changes Mar 22, 2022
View reviewed changes
@menghif
Copy link
Contributor Author

menghif commented Mar 22, 2022

I just noticed that the main banner image doesn't load on my machine. Does it work on your end? @cindyledev @DukeManh
Also Vercel deployment is stuck and I don't know how to restart it.

@DukeManh DukeManh dismissed their stale review March 22, 2022 19:59

Vercel deployment failing

@DukeManh
Copy link
Contributor

@humphd Could you please update Vercel build from src/web to src/web/app too.

@DukeManh
Copy link
Contributor

@menghif, It's loading on mine, there's new changes to the image service you might want to rebuild

tcvan0707
tcvan0707 previously approved these changes Mar 22, 2022
View reviewed changes
@menghif menghif requested a review from humphd March 22, 2022 20:58
humphd
humphd previously approved these changes Mar 22, 2022
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.

Thanks for splitting this out, really helpful.

src/web/app/next.config.js
@@ -37,12 +37,12 @@ const forwardToNext = (envVar) => {
};

// Try using .env in the root (legacy Telescope 1.0)
const legacyEnvPath = path.join(__dirname, '../..', '.env');
const legacyEnvPath = path.join(__dirname, '../../..', '.env');
Copy link
Contributor

Choose a reason for hiding this comment

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

Good job thinking of these...

@menghif menghif dismissed stale reviews from humphd, tcvan0707, and cindyorangis via 1ed330c March 22, 2022 23:06
@menghif
Copy link
Contributor Author

menghif commented Mar 22, 2022

Rebased. @humphd Do we need to make changes to Vercel or can this be merged now?

@menghif menghif self-assigned this Mar 22, 2022
DukeManh
DukeManh previously approved these changes Mar 22, 2022
View reviewed changes
tcvan0707
tcvan0707 previously approved these changes Mar 22, 2022
View reviewed changes
JerryHue
JerryHue previously approved these changes Mar 23, 2022
View reviewed changes
cindyorangis
cindyorangis previously approved these changes Mar 23, 2022
View reviewed changes
@JerryHue
Copy link
Contributor

Hmm... This is kind of problematic. Every PR that changes the front-end is going to conflict with this PR when it gets merged. There are currently 3 urgent PRs that change the front-end. Those PRs are #3282, #3281, #3211.

I suggest we merge those three tomorrow, and then immediately resolve the conflicts in this PR and immediately merge, so that other PRs that need to be started on the front-end can be done as soon as possible.

Otherwise, we would need to move this to release 3.0-alpha to avoid unnecessary conflicts and back-and-forth for several PRs.

@humphd
Copy link
Contributor

humphd commented Mar 24, 2022

What's our plan for landing this? Git should be able to deal with file rename/moves automatically when rebasing.

@menghif menghif dismissed stale reviews from JerryHue, tcvan0707, and DukeManh via 58d317a March 24, 2022 15:44
JerryHue
JerryHue previously approved these changes Mar 24, 2022
View reviewed changes
@menghif menghif requested a review from JerryHue March 24, 2022 16:09
@menghif menghif merged commit 6046273 into Seneca-CDOT:master Mar 24, 2022
@cindyorangis cindyorangis added this to the 2.9 Release milestone Mar 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JerryHue JerryHue JerryHue approved these changes

@AmasiaNalbandian AmasiaNalbandian AmasiaNalbandian approved these changes

@cindyorangis cindyorangis cindyorangis left review comments

@DukeManh DukeManh DukeManh left review comments

@tcvan0707 tcvan0707 tcvan0707 left review comments

@humphd humphd Awaiting requested review from humphd

Assignees

@menghif menghif

Labels
None yet
Projects
None yet
Milestone
2.9 Release
Development

Successfully merging this pull request may close these issues.
7 participants
@menghif @DukeManh @JerryHue @humphd @cindyorangis @tcvan0707 @AmasiaNalbandian