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

Added new exception page #864

Closed
wants to merge 9 commits into from
Closed

Added new exception page #864

wants to merge 9 commits into from

Conversation

faustinoaq
Copy link
Contributor

@faustinoaq faustinoaq commented Jun 16, 2018

Description of the Change

This PR adds a new exception page based on Phoenix one, see: phoenixframework/phoenix#1776

This PR will be required by next amber watch PRs

vokoscreen-2018-04-14_18-07-30

Alternate Designs

Keep basic error pages

Benefits

Possible Drawbacks

[x] Reload (watch) checkbox depends on new amber watch configuration (based on amber_reload.js without reload pipe)

@faustinoaq
Copy link
Contributor Author

Some screenshots 😍

screenshot_20180414_132630
screenshot_20180414_080548
screenshot_20180414_052340
0 0 0 0_3000_404 1

@faustinoaq faustinoaq requested a review from a team June 16, 2018 18:42
@faustinoaq
Copy link
Contributor Author

current error page specs by @marksiemers already cover this 👍

@faustinoaq faustinoaq added this to the Version 0.8.0 milestone Jun 21, 2018
@paulcsmith
Copy link

@faustinoaq Just finished up extracting this to a shared lib: https://github.com/crystal-loot/exception_page

You should be able to use it here. I removed the "watcher" stuff since it isn't always needed, but you can hook into the provided methods for customizing the buttons and the javascript that are in the README. Feel free to open PRs for stuff you need in case it doesn't cover everything!

@faustinoaq faustinoaq mentioned this pull request Jul 6, 2018
@faustinoaq
Copy link
Contributor Author

@paulcsmith Thanks you so much! Your shard looks very very nice, I gonna use it here now 👍

@eliasjpr
Copy link
Contributor

eliasjpr commented Jul 8, 2018

Lucky already has an exception page base leveraging on this code and Amber yet does not this is a bummer :(

@faustinoaq
Copy link
Contributor Author

@eliasjpr Yeah. no problem 😅 The new exception_page shard is pretty useful an easy to use, maybe we should close this and open a new PR, WDYT?

@robacarp
Copy link
Member

robacarp commented Jul 9, 2018

@faustinoaq given the recent migration of a lot of this code to crystal community repo, should this be closed or merged?

@eliasjpr
Copy link
Contributor

@faustinoaq I agree to use the new shard instead you can close this PR and open a new one implementing the shard.

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.

4 participants