feat(website): decrease white space on details page#6322
Conversation
There was a problem hiding this comment.
(just using this file to make a thread)
I don't quite get this code in that characters can differ so much in length between i and W. My instinct is that hardcoded values would make more sense? (Or in theory one can measure the actual width of each label, but I'm not sure it would be worth it)
There was a problem hiding this comment.
Im not sure I understand your comment - this should measure the width of each label I think?
There was a problem hiding this comment.
ah I see the ch is just a rough character size estimate so this might still be off - it has worked quite well for the organisms we have atm though and I think it is an improvement over the current layout, chatGPT also suggested - but Im not sure how costly it is to always compute this on page load before rendering:
const canvas = document.createElement('canvas');
const ctx = canvas.getContext('2d')!;
ctx.font = getComputedStyle(document.body).font;
const maxWidth = sections
.flatMap(({ rows }) => rows)
.filter((row) => row.type.kind === 'metadata')
.reduce((max, row) => Math.max(max, ctx.measureText(row.label).width), 0);
return maxWidth > 0 ? `min(${Math.ceil(maxWidth)}px, 50%)` : 'max-content';
There was a problem hiding this comment.
ok doesnt seem too costly - I can try this instead as it will be the actual, not estimated width
There was a problem hiding this comment.
Hey, so:
(A) Agree that this looks way better than the status quo, and it will be great to improve this one way or another - thanks for working on it!
(B) I still think it would make much more sense to hard-code this (or to allow configuring the width we want per instance). With this implementation if we add a single very long display name then we'll create a huge gap between the label and the display name for 90% of items - that doesn't seem like what we want
There was a problem hiding this comment.
If we go with B we will probably need to configure a set width per organism, which I guess still wont fully account for each sequence entry having potentially different metadata fields available. But if that is generally prefered Im happy to switch to that
There was a problem hiding this comment.
(Sorry to drop off the conversation) Basically my instinct is that we would be that we would be able to find a sensible level that gives good results across the board for PPX. When I look at your preview I find that widths look pretty similar across organisms and again I don't think it's a problem if the occasional field is wrapped (indeed I think that's good if it means there's a smaller gap between field name and value for most fields, but we're striking that balance wrong atm I agree!)
There was a problem hiding this comment.
CCHF multi-ref does have in general a larger width of metadata keys than the other organisms and will be a similar width for other mutli-segmented organisms when their segments have larger names than just L, M, and S - so I do think customization on an organisms is required
There was a problem hiding this comment.
I guess I just don't think worrying about e.g. 2 characters for the CCHF makes sense when the 21ch width is more characters than that different to the label that determines it it (if I've understood correctly, the blue box in this screenshot is 21ch and its set by the length of this label which is 21 characters):
![]()
But anyway, it's probably not worth spending more time discussing - I'm OK for this to get merged and us to come back to it in future if we want :)
There was a problem hiding this comment.
Going to an individual sequence page e.g. https://improve-details-page.loculus.org/seq/LOC_001ACVA.1 seems to be causing an internal server error for me, I had a look in argo CD and it looks like an astro issue with server side-rendering: ReferenceError: document is not defined
| const alignmentMaxWidth = getMaxLabelWidth(alignmentSections, ctx); | ||
| const alignmentLabelColWidth = | ||
| alignmentMaxWidth > 0 ? `min(${Math.ceil(alignmentMaxWidth)}px, 60%)` : 'max-content'; | ||
|
|
There was a problem hiding this comment.
I think the widths need to be computed in a useMemo, with a check for something like if (typeof document !== 'undefined')
There was a problem hiding this comment.
Tom and I have determined that using canvas leads to some odd page flickering and some widths are miscalculated for some reason so this doesnt seem like a good solution - I will revert to the average character width for now and we can see if someone finds a better solution
370bce6 to
58bb103
Compare
58bb103 to
6ea543d
Compare

Flagged by @rneher
Make the width of the metadata displayName dependent on the metadata keys and value.
The maximum width the displayName can be is 50% for the normal metadata fields, 60% for the
alignment and QCfields. The actual value is then calculated from the max width of all metadata displayNames (this is an approximation based on average character size - it works better than trying to calculate the true width as pre-rendering with astro is a mess).Screenshot
🚀 Preview: Add
previewlabel to enable