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

experimental: Head Slot Icons #4770

Merged
merged 5 commits into from
Jan 22, 2025
Merged

experimental: Head Slot Icons #4770

merged 5 commits into from
Jan 22, 2025

Conversation

kof
Copy link
Member

@kof kof commented Jan 21, 2025

Description

image image
  • additionally replaced blockquote icon
  • changed the order of the components in general

Steps for reproduction

  1. click button
  2. expect xyz

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

@johnsicili
Copy link
Contributor

Prevent multiple Head Slots on one page?

Seems if they both define the same info, they'll both print out.

Screenshot 2025-01-21 at 12 50 26 PM

Add component desc.. suggestion:

The Head Slot component lets you customize page-specific head elements (like canonical URLs) which merge with your site's global head settings, with Head Slot definitions taking priority over Page Settings. For site-wide head changes, use Project Settings instead.

Page Settings Meta

If we don't migrate this right away, should we at least conditionally show it only if existing meta is defined? Or nah?

@kof
Copy link
Member Author

kof commented Jan 21, 2025

I don't think its a problem to have multiple head slots, in the end its the same concept as slots cc @istarkov

@kof kof marked this pull request as ready for review January 22, 2025 00:40
@kof kof requested review from istarkov and johnsicili January 22, 2025 00:41
Copy link
Contributor

@johnsicili johnsicili left a comment

Choose a reason for hiding this comment

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

I think it's fine but will provide some feedback if you choose to do anything with it.

These three icons are confusing as they all show some part of a box but don't work well together:
Screenshot 2025-01-21 at 6 06 24 PM

Also can you add the component desc. I suggested one in the thread

@kof
Copy link
Member Author

kof commented Jan 22, 2025

Seems if they both define the same info, they'll both print out.

yes, though this is technically same as if you add 2 divs with the same text ... up to the user

@kof
Copy link
Member Author

kof commented Jan 22, 2025

added description, merging ...

@kof kof merged commit 627ab09 into main Jan 22, 2025
27 checks passed
@kof kof deleted the head-slot.staging branch January 22, 2025 14:48
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