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

✨ prefer smallFilename for data insights #4452

Merged
merged 5 commits into from
Jan 16, 2025
Merged

Conversation

ikesau
Copy link
Member

@ikesau ikesau commented Jan 14, 2025

Resolves #4416

image

Changes:

  • Adds a documentType property to the DocumentContext object
  • Adds a codepath in Image.tsx to always use the smallFilename (if defined) if the image is being used in a data insight
  • Updates the latest data insight block with a similar prefer-small logic
  • Updates CSS (one small update remains, pending clarification from Marwa)

@ikesau ikesau requested a review from rakyi January 14, 2025 23:59
@ikesau ikesau self-assigned this Jan 14, 2025
@owidbot
Copy link
Contributor

owidbot commented Jan 15, 2025

Quick links (staging server):

Site Dev Site Preview Admin Wizard Docs

Login: ssh owid@staging-site-single-image-data-insights

SVG tester:

Number of differences (default views): 0 ✅
Number of differences (all views): 0 ✅

Edited: 2025-01-15 00:12:53 UTC
Execution time: 1.21 seconds

Copy link
Contributor

@rakyi rakyi left a comment

Choose a reason for hiding this comment

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

Just to confirm, we want to retroactively apply this change for all existing data insights, right?

For the logic, one of my favorite maxims of software development is "I don't know, I don't wanna know". It would be better if the Image didn't know where it is used, and instead we passed in a prop (preferSmall?) to achieve the same effect. We always know we are in a data insight at the call site after all, right? That way it will stay more decoupled.

@ikesau
Copy link
Member Author

ikesau commented Jan 15, 2025

Yup, retroactive (hence why I contemplated this solution, but decided it was too much work for this somewhat-experimental project (even though I feel 80% confident that we'll stick with single-image data insights))

I like that maxim, and I feel you on the context codepath thing. preferSmallfilename is just such a stupid prop that we'd never use anywhere else. Prop-drilling it from DataInsightBody down to ArticleBlocks down to ArticleBlock down to Image seemed a little bit worse.

If you feel more strongly about it than I do, I'll gladly change it, though 🙂

@rakyi
Copy link
Contributor

rakyi commented Jan 15, 2025

I think adding it to the image block(s) in content body would be probably nicer than prop drilling it at the top level. But I prefer either of those to the context. So please change it, unless there is something problematic lurking there that we missed.

@ikesau ikesau force-pushed the single-image-data-insights branch from 09c6bcf to 950dc12 Compare January 15, 2025 22:31
@ikesau
Copy link
Member Author

ikesau commented Jan 15, 2025

@rakyi changed 🙂

Copy link
Contributor

@rakyi rakyi left a comment

Choose a reason for hiding this comment

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

Great, thank you!

@ikesau ikesau force-pushed the single-image-data-insights branch from 950dc12 to 201535c Compare January 16, 2025 14:29
@ikesau ikesau merged commit 5e8a4c1 into master Jan 16, 2025
17 of 19 checks passed
@ikesau ikesau deleted the single-image-data-insights branch January 16, 2025 15:27
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.

Cycle 2025.1: data insights with a single image
3 participants