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

Initial pass at printable report #89

Merged
merged 4 commits into from
Oct 1, 2024
Merged

Initial pass at printable report #89

merged 4 commits into from
Oct 1, 2024

Conversation

Pete-Y-CS
Copy link
Collaborator

I think the wording will consideration, and we will want to share the layout more intelligently between the report page and the printable report page, but this is is okay proof of concept to show Will & Jadene

import { criteria } from "../../lists";
</script>

<div class="govuk-width-container">
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should there at least be some kind of label / title at the top? There's a lack of context here. How about something like "Planning Application Assessment Toolkit Report"
image


<div class="govuk-width-container">
<table>
<caption class="govuk-table__caption govuk-table__caption--m">
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like all of this is exactly the same as the other report page. If so, a component to share between the two would be nice. The purpose of the new page is then just to discard some of the nav elements by using only the root layout

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I intended to do this, and just before opening your review remembered I didn't get around to it 🙈. On it

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably worth fixing and merging the other PR first, since there's changes there

import { base } from "$app/paths";

function openReportInNewTab() {
window.open(`${base}/planning/report/printable`, '_blank').focus();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd remove the .focus(). There's a TS error here, because open can fail. I think ?.focus() would fix it. But I tried without focus at all, and there's browser-dependant behavior to autofocus or not. In Firefox by default, it's a new tab that autofocuses. I expect that's probably fine for most users.

Peter York added 2 commits September 30, 2024 12:18
Reorder and reword

PR requests, needs rebasing when other PR is done
Reorder and reword

PR requests, needs rebasing when other PR is done

Npm run fmt and check

Initial pass at printable report

Reorder and reword

PR requests, needs rebasing when other PR is done

Npm run fmt and check
@Pete-Y-CS Pete-Y-CS force-pushed the printable-report-page branch from ac3ccbc to 9495b18 Compare September 30, 2024 11:19
@Pete-Y-CS
Copy link
Collaborator Author

Think this is sorted, seems to be working locally, formatted and checked

@Pete-Y-CS
Copy link
Collaborator Author

Happy to merge this one?

@Pete-Y-CS Pete-Y-CS merged commit acaaad6 into main Oct 1, 2024
1 check passed
@Pete-Y-CS Pete-Y-CS deleted the printable-report-page branch October 1, 2024 10:35
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.

2 participants