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

Change BrewRenderer background in Heroku test deployments #3688

Conversation

G-Ambatte
Copy link
Collaborator

@G-Ambatte G-Ambatte commented Aug 29, 2024

This PR modifies the BrewRenderer to change the default background when the HEROKU_APP_NAME variable exists. This variable only exists when created during the PR deployment process in Heroku apps, or when specifically set on a Development machine.

The main purpose of this PR is to create a visual indication that someone is working on a deployment, NOT the main website. This is primarily intended to be a Dev Experience QoL improvement, which is why I've marked it as a low priority change.

@G-Ambatte G-Ambatte added P3 - low priority Obscure tweak or fix for a single user 🔍 R0 - Needs first review 👀 PR ready but has not been reviewed experiment labels Aug 29, 2024
@G-Ambatte G-Ambatte self-assigned this Aug 29, 2024
@G-Ambatte G-Ambatte changed the title Change BrewRenderer background in Heroku deployments Change BrewRenderer background in Heroku test deployments Aug 29, 2024
@calculuschild
Copy link
Member

What issue is this trying to solve?

@5e-Cleric
Copy link
Member

What issue is this trying to solve?

This is trying to help us in the development, so we don't confuse local vs deployment vs live. Minor inconveniences.

@dbolack-ab
Copy link
Collaborator

Can we get a deployment on this one to validate?

Copy link
Collaborator

@dbolack-ab dbolack-ab left a comment

Choose a reason for hiding this comment

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

Looks good. Need to see the deployment.

Copy link
Collaborator

@dbolack-ab dbolack-ab left a comment

Choose a reason for hiding this comment

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

Looks good, deployment works great for the intended use.

@calculuschild
Copy link
Member

calculuschild commented Oct 4, 2024

@G-Ambatte Can you address 5e-Cleric's open comment and resolve conflicts, and then I can merge this.

@calculuschild calculuschild added 🔍 R3 - Reviewed - Awaiting Fixes 🔧 PR is okayed but needs fixes before merging and removed 🔍 R0 - Needs first review 👀 PR ready but has not been reviewed labels Oct 4, 2024
@G-Ambatte
Copy link
Collaborator Author

@G-Ambatte Can you address 5e-Cleric's open comment and resolve conflicts, and then I can merge this.

I've implemented the suggested change... which led me to investigate if state.height was actually being used anywhere after the change. As best I can determine it was not, so I have also removed the now obsolete state and event listeners from BrewRenderer.

@G-Ambatte G-Ambatte temporarily deployed to homebrewery-pr-3688 October 11, 2024 00:15 Inactive
@calculuschild
Copy link
Member

Nice added cleanup. Merging!

@calculuschild calculuschild merged commit 5f9dfc9 into naturalcrit:master Oct 11, 2024
2 checks passed
@G-Ambatte G-Ambatte deleted the experimentalDeploymentIdentification branch October 11, 2024 03:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
experiment P3 - low priority Obscure tweak or fix for a single user 🔍 R3 - Reviewed - Awaiting Fixes 🔧 PR is okayed but needs fixes before merging
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants