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

Migrate image tags #3855

Closed
1 task
Tracked by #3839
beechnut opened this issue Jun 5, 2024 · 12 comments
Closed
1 task
Tracked by #3839

Migrate image tags #3855

beechnut opened this issue Jun 5, 2024 · 12 comments
Assignees

Comments

@beechnut
Copy link
Contributor

beechnut commented Jun 5, 2024

Images are currently referenced mostly through <img> tags, but we should convert them to using image / image_with_class shortcodes, in keeping with 18F/guides convention.

Acceptance criteria

@beechnut
Copy link
Contributor Author

beechnut commented Jun 5, 2024

@SSPJ If you finish this and want more, #3858 is fairly related to this one.

@SSPJ SSPJ mentioned this issue Jun 7, 2024
7 tasks
@SSPJ
Copy link
Member

SSPJ commented Jun 7, 2024

@beechnut , @cantsin , and @bpdesigns I've opened #3863 for this.

There's about 100 left to do. (They seemed to make more as I went. 😅 ) Many of them are easy, but a few have conditional logic that I'm not sure how to interpolate. Since I'm staffed to a new project on Monday, I'll let it to the three of you to decide where to take this ticket and pull request.

@cantsin
Copy link
Member

cantsin commented Jun 7, 2024

Thanks! I think we can find someone else to pick up where this left off.

@beepdotgov
Copy link
Member

beepdotgov commented Jun 14, 2024

Just a small scope question: most of the raw <img> elements are inside posts. But since the acceptance criteria mentions layouts/partials specifically, I thought I'd check -- should I migrate images in posts as well?

Thank you!

@SSPJ
Copy link
Member

SSPJ commented Jun 14, 2024

@beepdotgov , if you are working from #3863 I've already done a fair number of the img tags in the posts, although I don't know whether I was supposed to--I just assumed we should use the template syntax everywhere--so I appreciate you posing the question.

@beechnut left a review comment about the CSS that is worth keeping in mind if you are doing the img tags in the posts. Some of the old images have width (or other) attributes which I converted to CSS classes because they don't map cleanly to USWDS utility classes. Ideally, if you had time or motivation, you could swap out that bandaid CSS with USWDS classes and do a visual inspection to make sure they still look "right".

@beechnut
Copy link
Contributor Author

@beepdotgov The goal is to replace all the image tags with the shortcodes, to align with the practice in 18F/guides. Feel free to do it in several passes — there's a lot of content to be replaced.

@beepdotgov
Copy link
Member

@SSPJ @beechnut Hey, thanks to both of you -- this is just the direction I needed. Really appreciate it!

@beechnut
Copy link
Contributor Author

@beepdotgov Based on our conversation, I'm going to revoke my last comment — this ticket is only to set up shortcodes in layouts and partials. We'll do blog post images post-migration in #3864.

@beepdotgov
Copy link
Member

@beechnut Today's chat was massively helpful. Thanks so much for talking through everything!

Working on getting the latest into my workspace now, and then I'll finish up the shortcode migration in the layouts/partials.

@beepdotgov
Copy link
Member

Okay! Per this morning's chat, I:

  • Pulled the full paths into our data files (03b653e)
    • (Nice side effect of making this change? This eliminated the need to bother with string interpolation in the new shortcodes, so we could skip the {% assign %} step!)
  • Migrated the remaining include/template <img/> elements over to use the shortcodes. (27f1973)

@beechnut Would it be helpful if I took a look at the open notes on the #3863 PR?

@beechnut
Copy link
Contributor Author

@beepdotgov Yes please! There are a few changes there that need to be reverted or otherwise reviewed.

@beepdotgov
Copy link
Member

Done, @beechnut! Left a question for you on one, and left another one open for review; everything else has been closed out.

Thanks again for all your help today!

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

No branches or pull requests

6 participants