Skip to content

Conversation

carlobeltrame
Copy link
Member

Fixes #4650
image

A5 / A6 (bigger relative font size): Nuxt A5 Client A5
A4 / A5 (normal, same as on prod): Nuxt A4 Client A4
A4 / A3 (smaller relative font size): Nuxt A3 Client A3

Please feedback on:

  • UI design of the new option
  • Currently, we use A4 page sizes but many users print this on A5. I took the easy way out and left the page size default at A4 (so I don't have to change all font sizes in both print) and just labeled it as "Normal (for printing on A4 or A5)" in the UI. So when the user selects the font size "Bigger (for printing on A6)", the PDF actually uses a page size of A5, and we rely on the users' printers to scale this down. Is this fine? Do users think more in terms of page size or in terms of font size? Should I do the tedious work and change all font sizes in both prints, so that we can be honest to the users?
  • Any bugs in the pdfs. I already had to adjust the picasso time column font size in nuxt print and the auto-scaling font size on picasso schedule entries in client print already.

@carlobeltrame carlobeltrame requested a review from a team August 26, 2025 10:58
@BacLuc
Copy link
Contributor

BacLuc commented Aug 26, 2025

The space problem for small blocks get worse on the picasso:

Nuxt A5:
image

And the 07:00 is over the line.

If for whatever reason you use 2 zeros for the relative time from block start, it always wraps:
image
I didn't write many blocks who were longer than 10h...?
But the problem persist for people who use absolute times.

Here it is completely off:
image

Material list looks good:
image

ThematicArea looks good:
image

Client A5
Picasso looks good:
image

Here the user can use 2 digit hours in the block:
image

A3:

I don't know what you would want to print on A3, maybe the picasso?
This works, looks a little empty.

UI Design:
Good that it's hidden and does not clutter the ui for not advanced users.
Not so discoverable...but you have to weigh the 2 options.

@@ -106,6 +106,10 @@ describe('repairConfig', () => {
},
},
]
const defaultOptions = {
Copy link
Contributor

Choose a reason for hiding this comment

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

❤️

})

const pageSizes = [('A5', 'A4', 'A3')]
pageSizes.forEach((pageSize) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Cool, nitpick:
With vitest you have to get used to it, but the syntax for parametrized tests is:

describe('idToColor', () => {
  it.each([
    [[undefined, false], '#4d4d4d'],
    [[null, false], '#4d4d4d'],
    [['', false], '#4d4d4d'],
    [['0000000', false], '#900'],
    [['0000000', true], '#4d4d4d'],
    [['fffffff', false], '#992600'],
    [['Wrong input', false], '#900'],
  ])('maps %p to %p', (input, expected) => {

See

as on example.
Or search "it.each"

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I've been in the ruby testing world a lot... e4d3c1f

})
})
})

Copy link
Contributor

@BacLuc BacLuc Aug 26, 2025

Choose a reason for hiding this comment

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

Maybe add: Repairs invalid page size

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea! Done in e4d3c1f

@@ -50,8 +51,15 @@ export default {
linkTarget() {
return `#scheduleEntry_${this.scheduleEntry.id}`
},
fontSizeScalingFactor() {
return {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a different way for a switch statement?

Copy link
Member Author

Choose a reason for hiding this comment

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

Replaced in e4d3c1f

@@ -36,6 +36,7 @@ const props = defineProps({
},
index: { type: Number, required: true },
filter: { type: Object, default: () => ({}) },
pageSize: { type: String, default: 'a4' },
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick:
In clientprint its 'A4', here 'a4'

Copy link
Member Author

Choose a reason for hiding this comment

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

That's actually intentional. In nuxt print, a4 is used as a CSS class name, and those are usually lowercase. In client print, A4 needs to be passed into the <Page> element, and it expects it to be uppercase.

@BacLuc
Copy link
Contributor

BacLuc commented Aug 26, 2025

What we also have to discuss:

How much value do we get vs additional maintenance effort?

Do we need tests for the other font sizes?
How do we test the other font sizes?

How much priority does it get when it breaks?

@BacLuc BacLuc requested a review from a team August 26, 2025 19:44
@carlobeltrame
Copy link
Member Author

carlobeltrame commented Aug 27, 2025

The space problem for small blocks get worse on the picasso

True, but if you are using this advanced feature, you probably can also set the picasso to portrait mode and maybe even split it in two parts using the date filters, so it's less of a problem.

And the 07:00 is over the line.

Fixed in 10724c5

I don't know what you would want to print on A3, maybe the picasso?
This works, looks a little empty.

How much value do we get vs additional maintenance effort?

I could also imagine dropping the A3 option, and just supporting the A6 one for now. I agree A4/A3 is only useful for the picasso, if at all. It does improve the "space problem for small blocks" which you mention on the A6 version, but nobody has explicitely asked for reducing the font size as far as I am aware (although when users claim they can create a better readable picasso in excel, this usually just boils down to a smaller font size).

I implemented this because I see the A6 variant as a valid use case for which we can't provide any workarounds at the moment. And if it's just setting the page size plus 1-2 workarounds for the smaller paper size, I feel like the maintenance burden is not so high.

Do we need tests for the other font sizes? How do we test the other font sizes?

I now added basic e2e tests just checking for the number of pages in the A6 variant. 28461de The e2e tests turned out to be extremely flaky in firefox for some reason... I removed them again and added snapshot tests in the pdf module (9c6cda3). In print, we don't have any logic tests at all so far...

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.

Add Print Layout for small paper formats
2 participants