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

Preload links & Custom Tags #300

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

sp4r74cus
Copy link

Added support for preload links:
https://developer.mozilla.org/en-US/docs/Web/HTML/Attributes/rel/preload

Also, to have the ability to add any other meta tag that is not yet supported i added a custom array that takes a raw html tag and adds it in the header.

@J-Brk J-Brk added the feature label Mar 16, 2023
@J-Brk J-Brk requested review from J-Brk and vinicius73 March 16, 2023 17:31
Comment on lines 204 to 207
// if $href is empty jump to next
if (empty($href)) {
continue;
}
Copy link
Collaborator

@J-Brk J-Brk Mar 16, 2023

Choose a reason for hiding this comment

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

I think this check is unnecessary: since the addPreload function already requires both href & as to be present.

Copy link
Author

Choose a reason for hiding this comment

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

yes, you are right. removing it.

@@ -18,6 +18,8 @@
* @method static \Artesaos\SEOTools\Contracts\MetaTags addKeyword(string $keyword)
* @method static \Artesaos\SEOTools\Contracts\MetaTags removeMeta(string $key)
* @method static \Artesaos\SEOTools\Contracts\MetaTags addMeta(array|string $meta, string|null $value = null, string $name = 'name')
* @method static \Artesaos\SEOTools\Contracts\MetaTags addPreload(string $href, string $as)
* @method static \Artesaos\SEOTools\Contracts\MetaTags addCustom(array|string $meta)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
* @method static \Artesaos\SEOTools\Contracts\MetaTags addCustom(array|string $meta)
* @method static \Artesaos\SEOTools\Contracts\MetaTags addCustom(string $meta)

Copy link
Collaborator

@J-Brk J-Brk left a comment

Choose a reason for hiding this comment

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

Thank you for very much for your contribution! I have reviewed your submit an found a few minor things that do need to be addressed. Please review then and adjust them where needed.

@sp4r74cus
Copy link
Author

updated the files as per your comments. Thanks!

@vinicius73
Copy link
Member

Very nice @sp4r74cus
Can you provide some tests for it?

@J-Brk J-Brk self-requested a review April 28, 2023 14:06
@J-Brk
Copy link
Collaborator

J-Brk commented Apr 28, 2023

Very nice @sp4r74cus Can you provide some tests for it?

If he doesn't make any tests; I will make them in a different commit; since I think this would be an great feature to add to make this package more universal.

@vinicius73
Copy link
Member

There is a conflict @J-Brk
Can you fix it?

Also, shold be good do the test with it.

Copy link
Member

@vinicius73 vinicius73 left a comment

Choose a reason for hiding this comment

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

Fix conflicts

@sp4r74cus
Copy link
Author

Dear @vinicius73 & @J-Brk,
I apologize for the delayed response. I was away from this project. Thank you for your work and efforts to make this update available!

@J-Brk
Copy link
Collaborator

J-Brk commented Jun 16, 2023

  • Conflicts resolved
  • Make unit tests

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants