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

feat: Add head component #4751

Merged
merged 27 commits into from
Jan 18, 2025
Merged

feat: Add head component #4751

merged 27 commits into from
Jan 18, 2025

Conversation

istarkov
Copy link
Member

@istarkov istarkov commented Jan 16, 2025

Description

Allows Head component

Hidden under headSlotComponent flag

Vercel fixture now have a HTMLRewriter example. Improves development.

Todo In Next PR

  • Head Title support

  • Icons

  • Allow reset as field

  • - BG is now white always

  • - It's shown as fixed element to not break layout

Implementation details

This component has more priority above PageSettings meta tags
e.g. If some tag exists in PageSettings and in HeadSlot the HeadSlot wins

Steps for reproduction

Create HeadSlot

image image

See og:title is not duplicated.

  • - Check Elements Panel
image

See no duplicated. All tags exists.

  • - Click Home.
    See no meta tags are in Elements panel from /head-tag page

  • - Click back button

image

See all meta is rendered and is not duplicated.

Code Review

  • hi @kof, I need you to do
    • conceptual review (architecture, feature-correctness)
    • detailed review (read every line)
    • test it on preview

Before requesting a review

  • made a self-review
  • added inline comments where things may be not obvious (the "why", not "what")

Before merging

  • tested locally and on preview environment (preview dev login: 0000)
  • updated test cases document
  • added tests
  • if any new env variables are added, added them to .env file

@istarkov istarkov marked this pull request as ready for review January 18, 2025 14:12
@istarkov istarkov requested a review from TrySound January 18, 2025 14:13
@johnsicili
Copy link
Contributor

Good stuff!! 🔥

IMO we should set a background to this and the XML page because when the theme is dark it's hard to read.

Also set to full width

Screenshot 2025-01-18 at 7 23 28 AM

@istarkov
Copy link
Member Author

Good stuff!! 🔥

IMO we should set a background to this and the XML page because when the theme is dark it's hard to read.

Also set to full width

Screenshot 2025-01-18 at 7 23 28 AM

div is already a full width.

For background I can set something like white with 50% opacity would be visible in most cases

@istarkov
Copy link
Member Author

Probably we need to show it as absolute element above content. So it will not break the existing layout

@istarkov
Copy link
Member Author

Background fixed now white

@istarkov istarkov requested a review from TrySound January 18, 2025 16:42
@istarkov istarkov merged commit 42af921 into main Jan 18, 2025
29 checks passed
@istarkov istarkov deleted the head.staging branch January 18, 2025 18:34
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.

3 participants