-
Notifications
You must be signed in to change notification settings - Fork 393
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: individualized content previews for DVC pages #1379
Conversation
Hi @MerlynMShelley! I see this is your first contribution: first, thanks for the fix! It looks good to me overall, but does need a few small changes. Also, TypeScript lint is failing:
Looks like you just have to add |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The TS issue will have to be fixed before merge, but everything else looks fine on read.
I'll also test this out to confirm it works.
Yes, it is my first contribution. Okay, I get it about reformating. Thank you for your support and guidance. |
@MerlynMShelley thanks for the contribution! I don't see this fixing #857 though. We only added some additional meat attributes, right? I would say it partially addresses #1119 (not completely fixing it either). Please, check some comments regarding var names, and it's not clear to me where do we get the value from, do we need some checks in place, etc |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, check some comments from me.
@shcheklein Thank you for the support and guidance; I will add the camelCase structure and also include the custom text+ logo as you mentioned to have a distinct preview on the slack channel. I was just curious to resolve the issue. I would refer to the links you mentioned by Matt to come up with the best solution. Thank you. |
Thanks for this @MerlynMShelley! Roger and Ivan are already reviewing so please follow their lead in this PR. I'll just watch. Some notes from me:
I also deployed this to https://dvc-landing-fix-hash-85-w68hes.herokuapp.com/ to check stuff |
Wait actually from my screenshot, the preview is indeed customized with the document's title, so this PR does seem to address #857 (as well as #1119 partially). Cc @shcheklein So if it's possible to also customize the snippet and/or image (per #857 (comment)) then the description of this PR is accurate, I think. |
BUT it's missing the 2nd half of the page title, "| Data Version Control . DVC", @MerlynMShelley: |
@MerlynMShelley a few questions:
|
I have referred this article on slack help, And I have performed site audit in these tools to find out what is missing in the site. https://www.seoptimer.com/dvc.org#recommendation https://seositecheckup.com/seo-audit/dvc.org https://suite.seotesteronline.com/seo-checker/aHR0cDovL0R2Yy5vcmc=/content Then I noticed that So I started by including the meta tags. Yes, I will check according to your guidance and fix the same. Thank you. |
@MerlynMShelley wrong Jorge 🙂 |
Oh I am sorry.🙂. Will note that in future. |
@MerlynMShelley any updates on this? Thanks |
Yes, @jorgeorpinel. |
@MerlynMShelley I meant I don't think you've addressed all of Ivan's feedback in #1379 (comment) which would be necessary to move this PR forward. As for the SEO report and your idea, sounds interesting but kind o unrelated to this PR. Please open a separate issue with the report and your proposal idea again so we can follow up there.
@rogermparent do you have any suggestions as for how this PR can be brought to a point were its mergeable, extracting the part Merlyn feels unable to contribute? Or perhaps provide specific guidance on what source code files to try editing? Thanks |
Ok, sure I follow what you have suggested @jorgeorpinel Thanks |
I don't find these Card Validator, |
Please find the SEO report of DVC.org, Thanks, @jorgeorpinel @ shcheklein |
@shcheklein I found Secondly, I am not pretty much sure about where the changes have to be made in the src code As I propose to work on the SEO project idea In GSoD '20, I find this issue to make my first contributions. Thank you. |
@shcheklein please also check this for |
Sorry guys, I've been busy on another project and haven't checked my GitHub notifications in a bit.
By making the PR mergeable, do you mean just solving the CircleCI issue blocking merge or bringing the PR up to a standard of quality meant for merging? for the former, here's the CI log:
To fix this, |
og:siteName does not exist on that page, did you mean og:site_name - you should fix the code them twitter:siteName does not exist. @MerlynMShelley I'm really sorry, we don't have capacity to iterate with that much involvement on this particular task :( I'm going to close this and if you are still interested in this ticket please try to make a new PR taking into account all the feedback I left, and suggestion @rogermparent put together. |
@MerlynMShelley thanks for this and sorry it became complicated. It had a great start.
The general feedback I can give you is that the contribution should be complete, not just starting changes that require the team to finish them. So if you don't feel comfortable with the code base, please focus on content for now. Feel free to contribute with research about SEO issues and add some info in the issues, but please try to make it digested, not raw screenshots of some schema reference without links — those are pretty difficult to figure out. Please try to be brief and specific. However, actual contributions (PRs) would be best so again, maybe check our issues labeled |
Fixes #857