Skip to content
This repository has been archived by the owner on May 25, 2020. It is now read-only.

Seo improvements #678

Merged
merged 12 commits into from
Nov 13, 2018
Merged

Seo improvements #678

merged 12 commits into from
Nov 13, 2018

Conversation

elbotho
Copy link
Member

@elbotho elbotho commented May 9, 2018 •
edited by inyono
Loading

Changelog

Added

  • Improved title in entities (e.g. automated " - lernen mit Serlo" suffix) (#678)

Changed

  • Show a subject-specific Open Graph meta image (e.g. for Facebook previews) (#678)
  • Expose a tenant-spefiic Open Search description file (#678)

For #436, #650 and #654 and also adding canonical link tag.

@inyono needs assets and new images in assets (see question in commits)

@elbotho elbotho mentioned this pull request May 9, 2018
Closed
@aeneasr aeneasr self-requested a review June 8, 2018 14:25
@aeneasr aeneasr self-assigned this Jun 8, 2018
@Knorrke Knorrke changed the base branch from master to development November 7, 2018 10:39
elbotho and others added 6 commits November 7, 2018 17:38
@elbotho @Knorrke
seo: title tag rework for #654
bec5def 
now title tags get generated in the normalizer.
not perfect, but should work.
@elbotho @Knorrke
closes #650
4e069a9 
Also see asset change!
@elbotho @Knorrke
Finishing #436
e233de6 
#436 should now be closed. Needs asset update (meta_serlo_mathe.png,
meta_serlo_nachhaltigkeit.png, meta_serlo_bio.png).

@inyono or @arekkas I created the function getCanonicalLink in the
`Normalizer.php` and added a `rel=‘canonical’` link to prevent
duplicate content. Could one of you rewrite the function to use the IDs
of the object to create short unique links?
@elbotho @Knorrke
cleanup
f268527
@elbotho @Knorrke
updated head_title for #654
6c4e803
@Knorrke
move type specific logic into adapters
8a19cc4
@Knorrke
Copy link
Member

Knorrke commented Nov 7, 2018

@inyono I've structured the code as discussed and rebased it onto the current development branch. There's still an issue though: The Meta-Description field is ignored now. Didn't get it to work atm, and probably won't look at it again until monday. Feel free to take a look, if you want.

inyono
inyono suggested changes Nov 8, 2018
View reviewed changes
Copy link
Member

@inyono inyono left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks 👍 Will look over that in detail in the next days, I only noted the thing with images so that I don't forget that ;)

Edit: And the CI works, yay, needs yarn format

src/module/Normalizer/src/Normalizer/View/Helper/Normalize.php Outdated Show resolved Hide resolved
inyono
inyono reviewed Nov 12, 2018
View reviewed changes
src/module/Normalizer/src/Normalizer/View/Helper/Normalize.php Outdated

switch ($subject) {
case 'mathematik':
case 'math':
Copy link
Member

Choose a reason for hiding this comment

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

IMHO: either we add English versions of the images or remove the behaviour for en.serlo.org for now.

Copy link
Member

Choose a reason for hiding this comment

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

@Knorrke: I removed the handling for subjects on en.serlo.org for now (i.e. the fallback image is used instead). Are you fine with that?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, that's fine I guess

inyono
inyono suggested changes Nov 13, 2018
View reviewed changes
Copy link
Member

@inyono inyono left a comment

Choose a reason for hiding this comment

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

Refactored some stuff and handled meta images. Regarding the thing we discussed yesterday: IMHO we should make metaDescription vs. description explicit, i.e. getDescription returns the descriptions while (a newly added) getMetaDescription returns the description for the meta tags. @Knorrke could you add that?

src/module/Normalizer/src/Normalizer/Adapter/AbstractAdapter.php Outdated Show resolved Hide resolved
src/module/Normalizer/src/Normalizer/Adapter/AdapterInterface.php Outdated Show resolved Hide resolved
src/module/Normalizer/src/Normalizer/Factory/NormalizeHelperFactory.php
@@ -38,7 +38,10 @@ public function createService(ServiceLocatorInterface $serviceLocator)
{
$normalize = new Normalize();
$normalizer = $serviceLocator->getServiceLocator()->get('Normalizer\Normalizer');
$instanceManager = $serviceLocator->getServiceLocator()->get('Instance\Manager\InstanceManager');
Copy link
Member

Choose a reason for hiding this comment

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

added by @inyono: pass InstanceManager to Normalizer so we get a tenant-aware open search

src/module/Normalizer/src/Normalizer/Normalizer.php Outdated Show resolved Hide resolved
src/module/Normalizer/src/Normalizer/View/Helper/Normalize.php
{
$lang = $this->getSubdomain();

if ($lang === 'de' || $lang === 'en') {
Copy link
Member

Choose a reason for hiding this comment

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

added by @inyono: tenant-aware open search

@Knorrke
Copy link
Member

Knorrke commented Nov 13, 2018

So like $normalized->getMetadata()->getMetaDescription()? Because $normalized->getMetadata()->getDescription() is already used

@Knorrke
Copy link
Member

Knorrke commented Nov 13, 2018

Ok, I just don't know how this should work... The meta description is only text, but the fallback is markdown and needs to be rendered. So I probably want to use toPreview($object) in that case. But I can't access that in the adapters...

@Knorrke
Copy link
Member

Knorrke commented Nov 13, 2018

It's done now from my side, thanks again for the help 👍

inyono
inyono approved these changes Nov 13, 2018
View reviewed changes
Copy link
Member

@inyono inyono left a comment

Choose a reason for hiding this comment

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

Thanks, awesome 👍 Also thanks for @elbotho for the initial work :)

@inyono inyono removed the request for review from aeneasr November 13, 2018 16:37
@inyono inyono merged commit 2c9da86 into serlo:development Nov 13, 2018
@inyono inyono mentioned this pull request Nov 13, 2018
Closed
This was referenced Nov 13, 2018
Closed
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@Knorrke Knorrke Knorrke left review comments

@inyono inyono inyono approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@elbotho @Knorrke @inyono @aeneasr