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

Issue #272 Add tagging support #324

Closed
wants to merge 25 commits into from
Closed

Issue #272 Add tagging support #324

wants to merge 25 commits into from

Conversation

q5sys
Copy link

@q5sys q5sys commented Aug 13, 2020

This requires two additional changes to config.toml which is covered by the following PR:
google/docsy-example#70

@google-cla
Copy link

google-cla bot commented Aug 13, 2020

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@q5sys
Copy link
Author

q5sys commented Aug 13, 2020

@googlebot I signed it!

@LisaFC
Copy link
Collaborator

LisaFC commented Aug 18, 2020

This looks great! Can you update config.toml for the Docsy user guide (nested in this repo) in your PR as well?

@LisaFC
Copy link
Collaborator

LisaFC commented Aug 18, 2020

Also can you (even screenshot) what the resulting list of tagged pages/articles looks like? Or possibly if you update config.toml in the Docsy user guide it will show up in the Netlify preview. I'm a little unclear on how it'll work in the site. Would be nice to add a usage example, as well as documenting it.

@q5sys
Copy link
Author

q5sys commented Aug 18, 2020

This looks great! Can you update config.toml for the Docsy user guide (nested in this repo) in your PR as well?

Sure thing, I'll get that added to the PR

Also can you (even screenshot) what the resulting list of tagged pages/articles looks like? Or possibly if you update config.toml in the Docsy user guide it will show up in the Netlify preview. I'm a little unclear on how it'll work in the site. Would be nice to add a usage example, as well as documenting it.

I've got it integrated into the new FreeNAS/TrueNAS Docs site, so you can see a customized example: https://docs.ixsystems.com/hub/sharing/smb/smb-share/ but I can spin up my local instance of Docsy and get a pure sample. On the FreeNAS site, we have Font Awesome integrated which is why we have the little tag graphic, that's an easy add for someone, but not something I think Docsy ships with integration for... though maybe I'm wrong on that. But I'm certain google's material design fonts have something similar that could be used if people wanted a little extra flair.
When you click on any Tag at the top of an article, you'll get linked to the taxonomy page which lists all articles with that tag along with a short summary.

This PR also does not include the tag cloud which you can see in the FreeNAS site, over on the right hand panel. That's an additional partial file and a single line to enable it. That's an optional thing that I can add to the PR as well if you'd like. It can be disabled by default and I can add to the docs how someone could enable that.

@LisaFC
Copy link
Collaborator

LisaFC commented Aug 24, 2020

Oh that looks super! Yes, an option to include the tag cloud would be really nice, I don't think all sites will want to include it by default but the option is great.

And we do ship with Font-Awesome (as a GIthub submodule), so you might be able to get the dinky graphic there.
https://github.com/google/docsy/tree/master/assets/vendor

@q5sys
Copy link
Author

q5sys commented Aug 25, 2020

Oh that looks super! Yes, an option to include the tag cloud would be really nice, I don't think all sites will want to include it by default but the option is great.

And we do ship with Font-Awesome (as a GIthub submodule), so you might be able to get the dinky graphic there.
https://github.com/google/docsy/tree/master/assets/vendor

👍 I was planning on working on that this past weekend, but things came up. I'll try to find some time this weekend to update the PR and add in the documentation, and add sample links and describe how they are implemented in that use case.

@q5sys
Copy link
Author

q5sys commented Sep 29, 2020

I haven't forgotten about this, life has just gotten in the way recently. I'll try to get this done as soon as there's a break in the chaos. :)

@LisaFC
Copy link
Collaborator

LisaFC commented Oct 1, 2020

No problem at all, these things happen! Looking forward to it when you have the time.

@q5sys
Copy link
Author

q5sys commented Nov 11, 2020

I added the tag cloud, and update the docs to explain how to add tags to an article and how to enable the tag cloud since its disabled by default. Two screenshots and a link to a live site with these implemented has been added to the site guidance markdown file.

@q5sys
Copy link
Author

q5sys commented Nov 11, 2020

Im not sure why the build is failing with a nil pointer dereference, I've got things set up the same way as I do on three separate sites and its working on all those. So I'm not sure if netlify is doing something a bit different than I'm expecting... or if something else is going on... but I've never seen that error.

image

@google-cla
Copy link

google-cla bot commented Dec 14, 2020

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

It was recommended that this be updated for this to build with netlify, attempting to do so.
@google-cla
Copy link

google-cla bot commented Dec 14, 2020

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

@marcelbaumann
Copy link

@googlebot I fixed it.

@alexivkin
Copy link

Waiting on this PR to make it through... any roadblocks before it is merged?

@narrenfrei
Copy link
Contributor

What's the current status of this PR?

I've also already the taxonomies in use and thought to make a PR of my version. Shall I wait until this PR is merged or is it better to make a second PR?

@q5sys
Copy link
Author

q5sys commented Feb 25, 2021 via email

@narrenfrei narrenfrei mentioned this pull request Mar 22, 2021
@narrenfrei narrenfrei mentioned this pull request Apr 7, 2021
@narrenfrei
Copy link
Contributor

I'm planning to work on this issue in the next days and make a updated PR. Since this PR lags a bit behind the master branch, it would be easier if I could create a completely new branch from the current master. (Unfortunately, I am not the git professional to get it right, so that all previous commits from this PR are retained).
@q5sys Is it OK for you, if I start a complete new branch without your previous changes?

@q5sys
Copy link
Author

q5sys commented Apr 16, 2021

@narrenfrei That's fine. The company I work for transitioned away from Docsy because of this and a few other issues. While we got tagging working, we don't like having to maintain our own fork of Docsy with this and other changes. It's been 11 months since I filled the initial bug report, and after no progress in a resolution, I went and did the work myself. We're 4 months out from it being a year at this point for the PR. IMHO, that's a bit to long for there to be little upstream effort put out to merge in improvements... either by merging and polishing, or helping guide a contributor how to get the code up to standards and then merge it in. And I'm not speaking just personally, but regarding PRs from others as well. Code should either be accepted or politely declined. If I, or my company, is going to put dev time towards improving another project, some resolution should be reached. Either accept the PR or decline. Just do something with it... anything... so everyone can benefit or so effort can be put towards improving another project.

I get it, devs are busy and they've got a lot to do... I'm not trying to rag on anyone. But when outside devs take their personal time to fix/improve another projects code... the project should at the very least respect that time and effort and make some decision about the PR.

Feel free to close this PR and my other PR. Hopefully you'll be able to get the issue fixed and code merged.

@gwatts
Copy link
Contributor

gwatts commented Apr 16, 2021

thanks for your work on this @q5sys - Seems like their was a communication disconnect.. I think on my part (as a non core dev on this project), I read into a prior comment that you were going to find the time to add that config feature and wrap it up.. I don't generally start adding work to other people's PRs unless they explicitly say "I don't have time to add that right now, but feel free to add a commit to this PR or start a new one" and then it fell off the radar after that :-(

I'd be happy to do a final pass cleanup (though sounds like @narrenfrei is also up for that!) - Based on prior comments it just needed a config option and for caching to be clarified.

Did the CLA get signed?

@q5sys
Copy link
Author

q5sys commented Apr 16, 2021

@gwatts No worries, yea I had hoped to get time, but I've got my own projects I'm behind on and that's with a language I understand. lol.
I see opensource development as a team effort, so I never fear someone else pitching in and improving my code. Afterall if I'm doing a PR into someone's elses branch with the expectation that it's going to be merged... I better well accept that someone might send a PR into my PR for me to merge.
From what I can tell everyone who touched anything signed the CLA.
What matters it the bug gets fixed and the functionality gets added. Who gets credit for the commits doesn't matter to me.

@gwatts
Copy link
Contributor

gwatts commented Apr 16, 2021

@q5sys Appreciate your pragmatic approach there; i agree with you - ultimately we're all after the same thing :-)

We'll get this finished up and merged - It's going to be a great addition i think!

@LisaFC
Copy link
Collaborator

LisaFC commented Apr 16, 2021

Agree with everything @gwatts said above, and I think I too had the same misunderstanding. Looking forward to this being added to the project!

As an aside, now that our project is getting bigger, we're looking (led by @emckean) at some governance improvements - this will include having more community members who can approve/merge to the repo and a more formalized prioritization of issues/new features, so hopefully fewer useful features will fall through the cracks and we'll keep those PRs moving!

@huehnerlady
Copy link
Contributor

huehnerlady commented May 28, 2021

Hi, is there any update on this? I would be keen to use it and so would love for it to get merged :)

@narrenfrei
Copy link
Contributor

Hi @huehnerlady, yes I've started a new PR #554 which is ready to merge (since about 5-6 hours). I'm only waiting for some feedback (#554 (comment)) and some issues with the google-cla bot have to been solved ;-)

@huehnerlady
Copy link
Contributor

Hi @huehnerlady, yes I've started a new PR #554 which is ready to merge (since about 5-6 hours). I'm only waiting for some feedback (#554 (comment)) and some issues with the google-cla bot have to been solved ;-)

@narrenfrei great, thanks for letting me know :)

LisaFC added a commit that referenced this pull request Jun 11, 2021
@huehnerlady
Copy link
Contributor

I was wondering if that is not already finished by the taxonomy support? 🤔

@marcelbaumann
Copy link

I was wondering if that is not already finished by the taxonomy support? 🤔

As far as I knew taxonomy is fully supported. I use it on my site for quite some time (see e.g https://tangly-team.bitbucket.io/blog/). Probably, Lisa should close the change request as duplicate.

@LisaFC
Copy link
Collaborator

LisaFC commented Jan 23, 2022

Fixed in #554

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.

7 participants