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

Add nginx default pages, #1846 #2345

Merged
merged 24 commits into from Dec 2, 2021
Merged

Add nginx default pages, #1846 #2345

merged 24 commits into from Dec 2, 2021

Conversation

Mte90
Copy link
Member

@Mte90 Mte90 commented Jan 27, 2021

Screenshot_2021-01-27 40x Error

#1846

Checks

  • I've updated the changelog.
  • I've tested this PR
  • This PR is for the develop branch not the stable branch.
  • This PR is complete and ready for review.

@update-docs
Copy link

update-docs bot commented Jan 27, 2021

Thanks for opening this pull request! Make sure CHANGELOG.md gets updated with this change, additionally any docs that need updated can be found at https://github.com/Varying-Vagrant-Vagrants/varyingvagrantvagrants.org

GitHub
The VVV docs and website. Contribute to Varying-Vagrant-Vagrants/varyingvagrantvagrants.org development by creating an account on GitHub.

@Mte90 Mte90 mentioned this pull request Jan 27, 2021
@Mte90 Mte90 marked this pull request as ready for review January 27, 2021 14:09
@Mte90
Copy link
Member Author

Mte90 commented Jan 27, 2021

As it is now there is no crash but I don't know how to test it and get a 403 as example.

@Mte90 Mte90 requested a review from tomjn January 27, 2021 14:10
Mte90 and others added 2 commits January 27, 2021 15:32
Co-authored-by: Tom J Nowell <contact@tomjn.com>
Co-authored-by: Tom J Nowell <contact@tomjn.com>
Mte90 and others added 3 commits January 27, 2021 15:32
Co-authored-by: Tom J Nowell <contact@tomjn.com>
Co-authored-by: Tom J Nowell <contact@tomjn.com>
Co-authored-by: Tom J Nowell <contact@tomjn.com>
@tomjn
Copy link
Member

tomjn commented Jan 27, 2021

Ideally we use css grid or flexbox to center the box vertically and horizontally, then give it a max width close to 600px

@Mte90
Copy link
Member Author

Mte90 commented Jan 27, 2021

I copied the css from the dashboard but is so minimal that page that is required to use flexbox?

@tomjn
Copy link
Member

tomjn commented Jan 27, 2021

not so much for minimalism but for aesthetics, making it centered vertically will be difficult without flex or grid

@tomjn tomjn added this to the 3.6 milestone Jan 27, 2021
@tomjn tomjn modified the milestones: 3.6, 3.7 Mar 8, 2021
@Mte90 Mte90 requested a review from evertiro April 30, 2021 09:32
@tomjn tomjn self-assigned this May 13, 2021
@tomjn
Copy link
Member

tomjn commented May 13, 2021

Just noting I want to make some wording changes to avoid general errors being misconstrued as VVV specific errors

@tomjn tomjn modified the milestones: 3.7, 3.8 Jul 27, 2021
@tomjn
Copy link
Member

tomjn commented Nov 29, 2021

I'm unable to get this to work, I turned off the PHP service and got this instead of the custom 502 page:

Screenshot 2021-11-29 at 19 05 40

@tomjn
Copy link
Member

tomjn commented Nov 29, 2021

@Mte90 I wanted to make changes to the pages themselves but I've not been able to get any of this to work, I don't know the relevant changes to the NGINX config to make this work and the various things i've tried on stackoverflow have made no difference

@tomjn
Copy link
Member

tomjn commented Nov 29, 2021

https://blog.adriaan.io/one-nginx-error-page-to-rule-them-all.html may hold the best approach it'd allow for a single html file too

I'm mindful right now all requests are handled by the dashboard or by sites, or custom extensions like tideways stuff

@tomjn
Copy link
Member

tomjn commented Nov 29, 2021

@Mte90 that article was what was needed, I refactored things and they work as expected, the easiest ways to test are:

Screenshot 2021-11-29 at 21 32 15

Screenshot 2021-11-29 at 21 24 40

I also removed references to VVV itself so that people don't assume this is our fault first, this is an Nginx generic HTTP code page, I also threw in 2 links to MDN and http statuscodes so they have somewhere to go to find out more

@tomjn
Copy link
Member

tomjn commented Nov 29, 2021

Minor styling changes and changelog updated:

Screenshot 2021-11-29 at 21 46 31

@Mte90
Copy link
Member Author

Mte90 commented Nov 30, 2021

we need a review from @evertiro or @msaggiorato

@tomjn
Copy link
Member

tomjn commented Nov 30, 2021 via email

Copy link
Member

@msaggiorato msaggiorato 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 after a new look at the code (I was originally confused about the error_page setting which i thought was only set in one of the server blocks, but it's actually in the http block, which superseeds all server blocks, so we're good.

@tomjn tomjn merged commit 590e3af into develop Dec 2, 2021
@Mte90 Mte90 deleted the mte90/1846 branch April 13, 2022 12:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants