-
Notifications
You must be signed in to change notification settings - Fork 120
Convert blog post titles to h1 while keeping the styles #475
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
base: main
Are you sure you want to change the base?
Conversation
CC: One more SEO task @AmandaPerino, @jathayde |
@bhumi1102 looking at the code we've got a few things to namespace to make this work I feel. So the headlines are defined as For these particular pages, the div is classed with How do you feel about that? |
Hi @jathayde - let me parse what you're saying, what do you mean by "then copy in the H2 items" above? In the diff, I've made updates to the post.scss file to add h1 styles. Though I think you're suggesting slightly different changes..
I consider all my changes placeholders to make it work. So I'm up for whatever you suggest as the person in charge of the rails website styles :) It's not clear to me what you're suggesting though, to help clarify what you mean, do you mind using the "suggest changes" directly on the diff to make your update? So I can see what you mean. (Also up for getting on a short call if that's faster/easier - I can be available Monday 5/19 anytime between 9am - 2pm central). Thanks! |
My apologies, let me step through it: In
Then, in order to not make them massively crazy, we need to modify the H1 but only for pages with a class of And then in
Only the |
No worries @jathayde! So the above makes sense and I made the h1 changes in Let me know if anything else in this PR. (I'd like to wrap up these SEO open PRs this week and merge if it works out). |
This looks good to me code wise. The one-off changes are the releases.html, etc? |
Yup, releases.html, news.html, foundations.html. Thanks for confirming @jathayde. This one should be good to merge I think. CC: @AmandaPerino |
Thanks for reviewing this one @zzak! No the blog titles are not suppose to shrink. They were missing the "desktop" styles, I've fixed this now! Attaching screenshot: ![]() Only change below is from h2 --> h1 (no font size change, as desired) ![]() |
To answer your second question @zzak, "releases" is used at ![]() |
@AmandaPerino I've addressed all review comments on this one. I was unsure about the font-sizes before but now I've sorted through the various media sizes and made sure that we are matching the existing sizes in all cases! |
This PR addresses this SEO task