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

chore: copy over demo files, lightly abstract #8

Merged
merged 1 commit into from
Jan 27, 2025
Merged

chore: copy over demo files, lightly abstract #8

merged 1 commit into from
Jan 27, 2025

Conversation

drewbo
Copy link
Contributor

@drewbo drewbo commented Jan 17, 2025

Changes proposed in this pull request:

  • copy over demo files, lightly abstract

Security considerations

None

@drewbo drewbo requested a review from a team as a code owner January 17, 2025 20:45
@drewbo drewbo requested a review from a team January 21, 2025 14:37
Dockerfile Outdated
@@ -0,0 +1,24 @@
FROM node:18.8-alpine as base

Choose a reason for hiding this comment

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

Should we switch to an lts version?

@@ -0,0 +1,20 @@
const SITE_URL =
process.env.NEXT_PUBLIC_SERVER_URL ||
process.env.VERCEL_PROJECT_PRODUCTION_URL ||
Copy link

@apburnes apburnes Jan 21, 2025

Choose a reason for hiding this comment

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

I think we can remove this platform specific env variable process.env.VERCEL_PROJECT_PRODUCTION_URL?

next.config.js Outdated
Comment on lines 5 to 7
const NEXT_PUBLIC_SERVER_URL = process.env.VERCEL_PROJECT_PRODUCTION_URL
? `https://${process.env.VERCEL_PROJECT_PRODUCTION_URL}`
: undefined || process.env.NEXT_PUBLIC_SERVER_URL || 'http://localhost:3000'

Choose a reason for hiding this comment

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

Could probably remove platform specific env vars like:

Suggested change
const NEXT_PUBLIC_SERVER_URL = process.env.VERCEL_PROJECT_PRODUCTION_URL
? `https://${process.env.VERCEL_PROJECT_PRODUCTION_URL}`
: undefined || process.env.NEXT_PUBLIC_SERVER_URL || 'http://localhost:3000'
const NEXT_PUBLIC_SERVER_URL = process.env.NEXT_PUBLIC_SERVER_URL || 'http://localhost:3000'

Copy link

@apburnes apburnes left a comment

Choose a reason for hiding this comment

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

This first run commit has a lot of boilerplate but added a few comments based on a few env vars that are not applicable to the cg platform. As we continue to flush out this editor experience, do you have any thoughts on how we can we incorporate unit and integration testing along with UI deviations?

With this initial prototype, do you have any concerns that could manifest into future impediments while managing and upgrading this product? Are they're an possible abstractions we could leverage to facilitate future development?

All together this is a exciting development with fairly low level of effort to prototype. This is 🥇 work.

@drewbo
Copy link
Contributor Author

drewbo commented Jan 24, 2025

As we continue to flush out this editor experience, do you have any thoughts on how we can we incorporate unit and integration testing along with UI deviations?

I haven't done a deep look into their test suite but purely by volume, I'd assume the built-in payload components are decently well tested. I think we should focus more on e2e testing with cypress/playwright to assure that our primary UX pathways do what we want

With this initial prototype, do you have any concerns that could manifest into future impediments while managing and upgrading this product? Are there any possible abstractions we could leverage to facilitate future development?

I'm going to ask @sknep to review a bit more because I do have a gnawing concern that there will be some awful UX or model pattern that we'll need to build a big workaround for but I haven't seen much of anything yet (and the one I did find can actually be fixed with a label change). I'm leaning more and more toward multi-tenancy which will make the initial build and compliance journey more difficult but I think will have a huge payoff when it comes to upgrading.

I'm going to add one more piece to this prior to merge (customizable Post type) just to map out that concept and then I think we can make more decisions about what to do with this

@drewbo drewbo requested review from apburnes and a team January 24, 2025 20:24
RUN rm -rf /tomcat/webapps/*

RUN wget https://github.com/qarik-group/uaa-war-releases/releases/download/v76.5.0/cloudfoundry-identity-uaa-76.5.0.war
RUN echo "99e3d5c166abc3400553d82022aca2caa921f4776d53d0402528f223fb02d521f7407789479b23489429fff4c6bd9382c8b31f324ebdc8c83dc55688d8668987 cloudfoundry-identity-uaa-76.5.0.war" | sha512sum -c
Copy link

Choose a reason for hiding this comment

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

this isn't a key to worry about, is it?


Content Editors can:
- Edit via WYSIWIG Editor
- Avoid login through Github
Copy link

Choose a reason for hiding this comment

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

not worth the edit but just in case you end up editing anyways:

Suggested change
- Avoid login through Github
- Avoid login through GitHub

@drewbo drewbo merged commit fd0ff35 into main Jan 27, 2025
@drewbo drewbo deleted the chore-demo branch January 27, 2025 15:46
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