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

[FCL-477] Handle judgements with no HTML content #1663

Merged
merged 15 commits into from
Feb 6, 2025

Conversation

jlhdxw
Copy link
Collaborator

@jlhdxw jlhdxw commented Dec 4, 2024

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:

localhost_3000_uksc_1701_999 (1)

localhost_3000_uksc_1701_999 (2)

image

Copy link
Contributor

@Terry-Price Terry-Price left a 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.

Copy link
Contributor

@Terry-Price Terry-Price left a 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.

Copy link
Collaborator

@jacksonj04 jacksonj04 left a 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.

ds_judgements_public_ui/templates/judgment/detail.html Outdated Show resolved Hide resolved
ds_judgements_public_ui/templates/judgment/detail.html Outdated Show resolved Hide resolved
ds_judgements_public_ui/templates/pages/atom_feed.html Outdated Show resolved Hide resolved
@jlhdxw jlhdxw force-pushed the FCL-477/handle-judgements-with-no-ncn branch from 82e7f59 to 197f0f9 Compare December 10, 2024 10:40
@jacksonj04 jacksonj04 changed the title Fcl 477/handle judgements with no ncn [FCL-477] Handle judgements with no HTML content Jan 29, 2025
@jlhdxw jlhdxw force-pushed the FCL-477/handle-judgements-with-no-ncn branch 3 times, most recently from e148f5a to 715a1c1 Compare February 3, 2025 11:59
@jlhdxw jlhdxw force-pushed the FCL-477/handle-judgements-with-no-ncn branch from 715a1c1 to 8a60b23 Compare February 4, 2025 15:02
@jlhdxw jlhdxw enabled auto-merge February 5, 2025 15:00
@jlhdxw jlhdxw added this pull request to the merge queue Feb 6, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 6, 2025
@jacksonj04 jacksonj04 added this pull request to the merge queue Feb 6, 2025
Merged via the queue into main with commit a326071 Feb 6, 2025
13 checks passed
@jacksonj04 jacksonj04 deleted the FCL-477/handle-judgements-with-no-ncn branch February 6, 2025 12:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants