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.

Sign up for GitHub

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

Jump to bottom

1258 updated base template <head> #1294

Merged
merged 7 commits into from
Mar 21, 2024
Merged

1258 updated base template <head> #1294

merged 7 commits into from
Mar 21, 2024

Conversation

jonathanbobel
Copy link
Contributor

@jonathanbobel jonathanbobel commented Mar 12, 2024 •
edited

Ticket: #1258

Description

I was tasked to create a base template for the document's

I've consolidated the various elements into one to use in the new base.html template

Changes Made

Consolidated the various to what should be on every page

Notes for the future

  • On a per-page basis, we could edit what we inject into the head going forward, like og description or other meta tags

@jonathanbobel jonathanbobel changed the title 1258 base template head 1258 updated base template <head> Mar 12, 2024
@jonathanbobel jonathanbobel marked this pull request as ready for review March 12, 2024 16:44
heyitsmebev
heyitsmebev requested changes Mar 12, 2024
View reviewed changes
Copy link

@heyitsmebev heyitsmebev left a comment •
edited

Choose a reason for hiding this comment

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

The head looks good ✅; however, there are extra files changed that go beyond the head template. I'm not sure if that was intentional or not. It looks like two of your PRs are included in this one, there could be a conflict once you merge the icon PRs

We'll need to note that wherever we're using the per_page_title block, it should be changed to pageTitle.

terrazoon
terrazoon approved these changes Mar 12, 2024
View reviewed changes
Copy link
Contributor

@terrazoon terrazoon left a comment

Choose a reason for hiding this comment

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

LGTM.

@jonathanbobel
Copy link
Contributor Author

The head looks good ✅; however, there are extra files changed that go beyond the head template. I'm not sure if that was intentional or not. It looks like two of your PRs are included in this one, there could be a conflict once you merge the icon PRs

We'll need to note that wherever we're using the per_page_title block, it should be changed to pageTitle.

Yeah I might keep this as draft until the other one is merged, as the other one touches the head with favicons so they are related

@jonathanbobel jonathanbobel marked this pull request as draft March 13, 2024 14:37
@jonathanbobel jonathanbobel marked this pull request as ready for review March 19, 2024 16:28
heyitsmebev
heyitsmebev approved these changes Mar 19, 2024
View reviewed changes
Copy link

@heyitsmebev heyitsmebev left a comment

Choose a reason for hiding this comment

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

✅ LGTM

ccostino
ccostino reviewed Mar 19, 2024
View reviewed changes
Copy link
Contributor

@ccostino ccostino left a comment

Choose a reason for hiding this comment

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

Just a couple of quick questions from me to confirm my understanding of things - if this is good to go then I'll finalize the review, I see others have approved!

app/templates/new/base.html Outdated Show resolved Hide resolved
app/templates/new/components/head.html Outdated Show resolved Hide resolved
@jonathanbobel jonathanbobel linked an issue Mar 21, 2024 that may be closed by this pull request
Base template: <head> #1258
Closed
2 tasks
@jonathanbobel jonathanbobel self-assigned this Mar 21, 2024
@ccostino ccostino merged commit 8e75110 into main Mar 21, 2024
8 checks passed
@ccostino ccostino deleted the 1258-base-template-head branch March 21, 2024 19:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ccostino ccostino ccostino left review comments

@terrazoon terrazoon terrazoon approved these changes

@heyitsmebev heyitsmebev heyitsmebev approved these changes

Assignees

@jonathanbobel jonathanbobel

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Base template: <head>
4 participants
@jonathanbobel @ccostino @terrazoon @heyitsmebev