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

Seo breadcrumbs #67

Merged
merged 9 commits into from
Feb 20, 2021
Merged

Seo breadcrumbs #67

merged 9 commits into from
Feb 20, 2021

Conversation

i7d3v3l0p3r
Copy link
Contributor

@i7d3v3l0p3r i7d3v3l0p3r commented Feb 17, 2021

Remade breadcrumbs that will work correct with Google Search and successful passed Google Search Live Test
image

fix: replace markdownify with renderString to use render hooks (thegeeklab#65)
@thegeeklab-bot
Copy link

thegeeklab-bot commented Feb 17, 2021

LHCI Report Overview

Report URL Performance Accessibility Best-Practices SEO PWA
Link http://localhost:40231/ 79 % 100 % 100 % 96 % 25 %
Link http://localhost:40231/404.html 80 % 100 % 100 % 100 % 25 %
Link http://localhost:40231/posts/ 73 % 100 % 100 % 100 % 25 %

@xoxys
Copy link
Member

xoxys commented Feb 20, 2021

Thanks for your contribution. I've added some smaller adjustments:

  • cleanup styling
  • replace deprecated itemtype="http://data-vocabulary.org/Breadcrumb"
  • keep surrounding <div> on mobile view to avoid content shift

@i7d3v3l0p3r Could you review my changes?

<div class="gdoc-page__header flex flex-wrap justify-between{{ if not $showEdit }} hidden-mobile{{ end }}{{ if (and (not $showBreadcrumb) (not $showEdit)) }} hidden {{ end }}" itemscope itemtype="http://data-vocabulary.org/Breadcrumb">
<span>
<div class="gdoc-page__header flex flex-wrap justify-between{{ if not $showEdit }} hidden-mobile{{ end }}{{ if (and (not $showBreadcrumb) (not $showEdit)) }} hidden {{ end }}" itemprop="breadcrumb">
<div>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@xoxys Maybe move div under {{ if $showBreadcrumb }} to prevent empty div block in DOM when $showBreadcrumb == false?

Copy link
Member

Choose a reason for hiding this comment

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

I've moved it back to prevent content shift if breadcrumbs are disabled. The empty div is required for the flex box layout to keep the "Edit page" link on the right side.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

and for mobile device change justify-between to justify-content: flex-end?

Copy link
Member

Choose a reason for hiding this comment

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

It's not just the mobile view, but should work now without the empty divs for both showBreadcrumb and showEdit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@xoxys thanks! 👍

@xoxys xoxys merged commit e29fa7a into thegeeklab:master Feb 20, 2021
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.

None yet

3 participants