-
Notifications
You must be signed in to change notification settings - Fork 6
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
[FCL-477] Handle judgements with no HTML content #1663
Conversation
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.
Hi James,
This is looking good. There are a few things I'd like to update:
The notification component, can we use the Home Office blues for this, I know it's another two blue, but I think that will help it stand out as it should. Perhaps you can label these colours as such so the are not used for anything else.
Can the width of the notification be set to the width of container. it will help keep it shallower if the messages get longer. See style guide for blues and width.
On the body text above the download button, can this use CSS balance, so it avoid widows. Same for the Web archiving text please.
The download boxes look good, will there be a Document summary tabel below them, I think that's the expectation.
You may have done this already, so apologies if you have. To apply an Aria label to the download buttons to provide context for screen readers.
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.
Thanks James.
This looks good, When the lighter grey is in it will be perfect.
As for the web archiving link, I know it's not in yet, but if we can utilise CSS balance on that, it would be less ugly than it breaks now.
thanks again for this extra mile effort.
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.
We need to have a bit of a re-think on exactly how we structure the logic here, and move even further away from the idea of a "PDF Only" page.
Ultimately, we need to be in a position where:
- A document may or may not have a human readable identifier
- The human readable identifier may or may not be an NCN
- The document may or may not have HTML content which can be rendered
- The document may be downloadable in 0-n possible formats, one of which may be a PDF (but equally might not)
All the bits are there, we just need to shuffle some logic.
82e7f59
to
197f0f9
Compare
e148f5a
to
715a1c1
Compare
715a1c1
to
8a60b23
Compare
Changes in this PR:
Adding a view for PDF only documents. Doesn't include adding the new details to the summary information as we don't have that yet, and the web archive link (have wrapped in a conditional statement checking for an attribute that doesn't exist)
Jira card / Rollbar error (etc)
https://national-archives.atlassian.net/browse/FCL-478
https://national-archives.atlassian.net/browse/FCL-481
https://national-archives.atlassian.net/browse/FCL-477
https://national-archives.atlassian.net/browse/FCL-480
Screenshots of UI changes: