Skip to content

Conversation

@jtfell
Copy link
Contributor

@jtfell jtfell commented Apr 17, 2025

No description provided.

@jtfell jtfell requested a review from AshKyd April 17, 2025 00:04
Copy link
Member

@AshKyd AshKyd left a comment

Choose a reason for hiding this comment

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

Approved in principle.

But what's going on with the whitespace? It looks like the source was in tabs and you've committed spaces. Not sure why Prettier is configured to use tabs, and I'm happy to change it, but I wanted to raise it with you first

image

color: var(--panel-color);
border-radius: var(--panel-radius);
padding: var(--panel-padding);
max-width: 640px;
Copy link
Member

Choose a reason for hiding this comment

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

Where did this come from?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ben said: "Tablet text line length should max out at 640px" - Perhaps we need to check with him if that's for only that one breakpoint? I suspect that it's a readability thing that applies in general

Copy link
Member

Choose a reason for hiding this comment

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

Gotcha. Let's check, because it might make it a bit tight on large desktop. Also let's convert it to rems, that's what the rest of the stylesheet is using

if ($isMobileRowMode) {
// For row layout on small portrait screens, block out space taken up by the viz at the top
const threshold = ($vizDims.dims[1] / $screenDims[1]) * 100;
// console.log($vizDims, $screenDims, threshold);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// console.log($vizDims, $screenDims, threshold);

import acto from '@abcnews/alternating-case-to-object';
import { selectMounts, isMount, getMountValue } from '@abcnews/mount-utils';
import type { PanelAlignment, PanelDefinition, ScrollytellerDefinition } from './types.js';
import type { PanelDefinition, ScrollytellerDefinition } from './types.js';
Copy link
Member

Choose a reason for hiding this comment

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

If we're not using PanelAlignment let's delete it from types.js too

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