-
-
Notifications
You must be signed in to change notification settings - Fork 229
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
🎉 (admin) data insight index page / TAS-845 #4490
base: gdoc-di-props
Are you sure you want to change the base?
Conversation
Quick links (staging server):
Login:
SVG tester:Number of differences (default views): 0 ✅ Edited: 2025-01-27 09:54:56 UTC |
439c38d
to
3588f43
Compare
ad87f26
to
8fed3de
Compare
3588f43
to
38a0ed4
Compare
8fed3de
to
a4a7190
Compare
46f4507
to
3d44a47
Compare
3d44a47
to
8698eef
Compare
38a0ed4
to
35a2035
Compare
8698eef
to
93c8378
Compare
35a2035
to
9004e2f
Compare
93c8378
to
55e7517
Compare
9004e2f
to
aed91a1
Compare
55e7517
to
8b38a95
Compare
aed91a1
to
54b0129
Compare
8b38a95
to
d132fba
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really nicely done!
I've left quite a few comments but none of them are blocking, I don't think. I do think it'd be nice to get EditableTags
working, though I realize that'll probably be a headache to fit in the table. 🙂
dataIndex: "tags", | ||
key: "tags", | ||
render: (tags: DbPlainTag[]) => | ||
tags.map((tag) => ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we use EditableTags
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, that's a good idea! it hadn't crossed my mind
dataInsight: RequiredBy<OwidGdocDataInsightIndexItem, "image"> | ||
) { | ||
const { cloudflareId, originalWidth } = dataInsight.image | ||
return `${CLOUDFLARE_IMAGES_URL}/${cloudflareId}/w=${originalWidth}` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May as well make this w=530
🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
somehow I thought this has be to be the original width, lol
LEFT JOIN chart_configs cc_chartView | ||
ON cc_chartView.id = cw.chartConfigId | ||
|
||
-- join the images table by filename (only works for data insights where the image block comes first) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think any author has ever not done this, but this will be the first time we explicitly hinge business logic on this assumption - so maybe we should now add validation to assert this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that's a good idea! I was debating whether it's okay to make this assumption here. I finally settled on this because writing more code to cover cases outside our guidelines didn't make much sense to me. Good call to make it explicit!
Thanks for the review, @ikesau! Making the tags editable is a very good idea, and I hope dropping the chart type column helps to make it fit :) |
54b0129
to
24daafe
Compare
d132fba
to
91bdd81
Compare
Adds an index page for data insights in the admin.
Summary
Notes
grapher-url
, I added some validation for it. As far as I know, thegrapher-url
should be a valid grapher or explorer link, but maybe I'm forgetting something?Screenshots
This is part 2 of 3 in a stack made with GitButler: