nephila / djangocms-page-meta

OpenGraph, Twitter Card and Google+ snippet tags for django CMS 3 pages
https://djangocms-page-meta.readthedocs.io/
BSD 3-Clause "New" or "Revised" License
59 stars 64 forks source link

remove google plus-related meta #117

Closed corentinbettiol closed 4 years ago

corentinbettiol commented 4 years ago

Since Google+ no longer exists, I think we can remove gplus meta tags (title & description).

yakky commented 4 years ago

@corentinbettiol thanks, I agree it's time to remove g+ tags Currently your PR includes a lot of unrelated changes that makes very difficult to review Could you please limit the diff to the required changes only?

corentinbettiol commented 4 years ago

@yakky oops :/

I removed all double quote chars, it should be clearer now :)

corentinbettiol commented 4 years ago

As it took much more time than expected, I'm going to move on to another project for a few days, maybe I'll come back next week :)

yakky commented 4 years ago

As it took much more time than expected, I'm going to move on to another project for a few days, maybe I'll come back next week :)

no worries, take your time!

corentinbettiol commented 4 years ago

(anyway I pushed the last changes I made, that replaced gplus_type by item_type and added schema_html_scope in templatetag)

codecov[bot] commented 4 years ago

Codecov Report

Merging #117 into develop will decrease coverage by 0.56%. The diff coverage is 78.94%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #117      +/-   ##
===========================================
- Coverage    88.77%   88.20%   -0.57%     
===========================================
  Files            8        8              
  Lines          374      373       -1     
  Branches        47       46       -1     
===========================================
- Hits           332      329       -3     
- Misses          31       32       +1     
- Partials        11       12       +1     
Impacted Files Coverage Δ
djangocms_page_meta/admin.py 95.45% <ø> (ø)
djangocms_page_meta/utils.py 81.65% <75.00%> (-1.99%) :arrow_down:
djangocms_page_meta/models.py 85.14% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 8be3e45...598a569. Read the comment docs.

corentinbettiol commented 4 years ago

I saw your commits on django-meta (great work!), and decided to adapt my changes to yours, so that the two modules can be updated at the same time.

yakky commented 4 years ago

I saw your commits on django-meta (great work!), and decided to adapt my changes to yours, so that the two modules can be updated at the same time.

You have been quicker than me :) I was planning to update here with the django-meta roadmap to coordinate the work

corentinbettiol commented 4 years ago

Do you see anything else to update in this branch ?

My next goal is to add tests to my branch that adds the robots meta tag.

coveralls commented 4 years ago

Coverage Status

Coverage decreased (-0.3%) to 91.512% when pulling 598a5691cac5538673fc082455d8592198a19b6a on kapt-labs:remove-gplus into 8be3e45c6be127e7915f26cac185df3e95c13222 on nephila:develop.

corentinbettiol commented 4 years ago

Here are all the schema.org properties currently implemented:

image

yakky commented 4 years ago

@corentinbettiol sorry it took me so much time to respond

I resolved most of the comments, I only left two, and then it's ready for merge

corentinbettiol commented 4 years ago

@yakky No problem, I solved a comment (the one about tox.ini), and added the properties you requested. The last thing to do is to tell me if I should display all properties all the time or only if the type is WebPage.

corentinbettiol commented 4 years ago

Hmm, I used the SCHEMAORG_TYPES choices list defined in https://github.com/nephila/django-meta/commit/1c3cffde9c813f29c522beb2a77571d53147e44e, but I just saw that this commit drop support for Python2 projects.

Maybe I can add a SCHEMAORG_TYPES choices list in djangocms-page-meta, so that it can still be used in Python2 projects?

yakky commented 4 years ago

Hmm, I used the SCHEMAORG_TYPES choices list defined in nephila/django-meta@1c3cffd, but I just saw that this commit drop support for Python2 projects.

Maybe I can add a SCHEMAORG_TYPES choices list in djangocms-page-meta, so that it can still be used in Python2 projects?

My plan is to drop python 2 in this repository too before the release and just depend on django-meta 2 I don't think python 2 it's worth the extra effort

corentinbettiol commented 4 years ago

Okay. This is a bit out of context, but since the creation of the meta robots tag property (#116) is derived from this branch, does that mean that adding this robots feature will also only be compatible with Python 3?

yakky commented 4 years ago

Okay. This is a bit out of context, but since the creation of the meta robots tag property (#116) is derived from this branch, does that mean that adding this robots feature will also only be compatible with Python 3?

Unless there is a strong reason to still support python 2, I think it's the best approach

yakky commented 4 years ago

Okay. This is a bit out of context, but since the creation of the meta robots tag property (#116) is derived from this branch, does that mean that adding this robots feature will also only be compatible with Python 3?

Unless there is a strong reason to still support python 2, I think it's the best approach

corentinbettiol commented 4 years ago

Okay, let's dump Python 2 :)

Is the last request solved?

yakky commented 4 years ago

@corentinbettiol model changes are good now, thanks

There is only the minor adaptation in the tests needed :bow:

yakky commented 4 years ago

@corentinbettiol thanks a lot for your huge efforts!

Merging this! :tada:

corentinbettiol commented 4 years ago

Thank you for your patience :D

Now I will finish the meta robots feature, and then submit a new PR :)