-
Notifications
You must be signed in to change notification settings - Fork 296
Cleanup of national map references and no longer used files #754
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
Conversation
df63414
to
76f5095
Compare
76f5095
to
4c0d128
Compare
|
||
<h2>Internal server error</h2> | ||
<p>Something went wrong on our server. Please send an email to <a href="mailto:data@pmc.gov.au">data@pmc.gov.au</a>, explaining what you were doing at the time it happened. This will help us track down the cause.</p> | ||
<p><b>← Return to <a href="/">NationalMap</a></b>. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this line be retained so the user has something to click on to get to somewhere that at least doesn't give an error page? I guess the destination should be made more generic though, perhaps just calling it "the map"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that we want to leave the design and the content decisions about 400.html and 500.html pages up to a particular implementation, so it makes sense to keep them very minimal.
<body> | ||
<h1 class="error-number">500</h1> | ||
<div class="inline-block align-middle"> | ||
<h2 class="error-description" id="desc">Internal server error.</h2> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think headings are typically without punctuation like periods/question marks/etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my comment for the 400.html page.
<p>You can perform these steps in any order as required to tune your display of spatial data.</p> | ||
|
||
|
||
<h2><a class="anchor" name="feature-info">How to find out about a displayed feature</a></h2> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some parts of this file, like this subsection, seem generally useful for a novice user. Is the information already available elsewhere in the UI?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be done in the future if needed.
As far as I understand it, the present PR is about cleaning all up.
<body> | ||
<h1 class="error-number">404</h1> | ||
<div class="inline-block align-middle"> | ||
<h2 class="error-description" id="desc">Not found.</h2> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment about no period in heading and providing a clickable link to somewhere reasonable as for the 500 page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Tested locally, everything worked well.
- Code changes LGTM.
- Ran
yarn prettier-check
and got warnings for a couple of files (see below). - I suggest adding
wwwroot/version.json
to.prettierignore
. - Running
yarn prettier
will fixlib/Styles/variables-overrides.scss
(remove a space).
[warn] lib/Styles/variables-overrides.scss
[warn] wwwroot/version.json
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps, we can ignore the prettier-check
here.
Approved.
No description provided.