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

Add tidelift_url customization option #142

Merged
merged 1 commit into from
Oct 2, 2018

Conversation

sloria
Copy link
Contributor

@sloria sloria commented Sep 6, 2018

Allows adding a Tidelift section to the sidebar
by setting the tidelift_url theme option.

Screenshot

screenshot

Allows adding a Tidelift section to the sidebar
by setting the `tidelift_url`theme option.

**Screenshot**

![screenshot](https://user-images.githubusercontent.com/2379650/45132398-cf6efe00-b15e-11e8-887b-6bf8e41e2f87.png)
@bitprophet
Copy link
Collaborator

bitprophet commented Sep 6, 2018

Thanks! Need this myself (🎉). Off the cuff thoughts follow; FWIW I can do any actionables myself if you don't get to them before I merge, which will prob be by next week on the outside.

  • When we add these third party integrations, esp ones with a single URL/service (vs federation or w/e) we prefer to go with sub-URL settings (e.g. Github slug and/or user/project, instead of full URL to github, when activating the Github corner banner).
    • However, I just looked at an example Tidelift project link (i.e. what they recommend when you're logged in as a lifter) and they are complex enough (and platform-specific enough) that my instinct is actually wrong here!
    • For example, Fabric's README-centric link would be https://tidelift.com/subscription/pkg/pypi-Fabric?utm_source=pypi-Fabric&utm_medium=readme:
      • tidelift.com/subscription/pkg/ is probably factorable enough
      • the base slug pypi-Fabric is going to differ across language/platform (eg a JS lib might be npm-leftpad)
      • the utm_source GET param seems to be identical to the slug always
      • the utm_medium may differ depending on where the user is coming from (readme, docs etc) - however since this PR is for a docs theme, we can presumably safely make this statically docs
    • I'm on the fence now, there's some pros and cons:
      • Since we can presumably just configure the slug (eg tidelift_slug = "pypi-Fabric") that would be acceptable shorthand.
      • OTOH, if at any point those other params change, or Tidelift changes their overall URL structure (They are still in beta), then having a straight URL would be more flexible
      • Could put in code for both, at expense of having more stuff to maintain...
  • Thought there was some badging or imagery involved in these links too...?
    • Seems like there's a separate, possibly optional dependency-status badge we could also add (but that probably falls under Add generic badges support #120).
    • There's also some generic assets for fancy-pants static-ish websites, which is what I was really thinking of, but that doesn't apply here unless we want to default to having a 'nice' piece of Tidelift art in the sidebar (vs a text-only link).
    • This can be punted on IMO.

OK well that's a lot of words for "we can probably merge this as-is and it wouldn't be the worst thing in the world". If Tidelift was more mature (not a dig, just a fact!) and I had more faith in the URLs not potentially changing, I might press harder on the slug-vs-url thing, but for now I suspect URL is most pragmatic.

Not like conf.py files are terrifically frequently read/written anyways so being terse isn't a huge benefit.

@bitprophet
Copy link
Collaborator

Reckon we need a changelog entry for this too but that's probably it.

@bitprophet bitprophet added this to the 0.7.11 milestone Sep 6, 2018
@sloria
Copy link
Contributor Author

sloria commented Sep 6, 2018

we prefer to go with sub-URL settings

Yeah, I saw that with github_user, github_repo, etc. But--as you pointed out--in Tidelift's case, the project name won't suffice. tidelift_slug = "pypi-Fabric" could work, but I'm not sure that those are stable identifiers.

OTOH, if at any point those other params change, or Tidelift changes their overall URL structure (They are still in beta), then having a straight URL would be more flexible

My thoughts exactly.

Thought there was some badging or imagery involved in these links too...?

Yeah, I experimented a bit with using the Tidelift logo but was unsatisfied with the way it responded to different screen sizes. I also didn't want the section to take too much screen real estate/visual weight.

Reckon we need a changelog entry for this too but that's probably it.

Would you like me to add that or would you prefer to add it after this is merged?

@bitprophet bitprophet modified the milestones: 0.7.11, 0.7.12 Sep 6, 2018
@bitprophet
Copy link
Collaborator

Would you like me to add that or would you prefer to add it after this is merged?

I can do it, I'm picky about format/language anyhow.

@bitprophet
Copy link
Collaborator

bitprophet commented Oct 2, 2018

Back on this now. Not having memorized the discussion, I'd been assuming we'd want at least an option for the graphical Tidelift logo, but I see you already experimented with that and found it doesn't necessarily scale super well. I think my earlier decision of "let's just go text only for now" is the right one, yea.

If Tidelift or users come back later with "the image would be so nice!" then we can work with them on finding an easy way to make sure the image is going to scale up/down/work on HiDPI displays/whatever.


New to the discussion: I'm not sure I like the "professionally supported" language. It is one of the pre-supplied snippets Tidelift recommends, but in my mind it's going to mislead folks who think that "sign up as a subscriber" == "maintainers are going to prioritize all their tickets".

OTOH, that's arguably between the subscribers and Tidelift, and Tidelift has already shown they're aware of the need to educate subscribers on exactly what assurances they are actually receiving, so...eh.

I'll play around with it and if I do change it, it will fall under the same policy as the logo: folks who don't like what gets added now can have a later discussion about making it more flexible.

@bitprophet
Copy link
Collaborator

Side issue, this looks mildly fugly on my display because of how donation.html is set up - there's nothing adding any padding or margin above the <h3> and so it runs into the search box too much.

This problem predates this PR (it would've happened for the Gratipay stuff too, which was rarely used and certainly shouldn't be in much use nowadays; eg I turned it off on my sites after they a) supported Nazis and b) then went bust anyways). But I might shoehorn a fix in anyways; it's not likely to break anything for anybody.

@bitprophet
Copy link
Collaborator

Think I might just opt to merge some variant of #143 also and then handle the styling thing after that.

@bitprophet bitprophet merged commit 759549d into sphinx-doc:master Oct 2, 2018
bitprophet added a commit that referenced this pull request Oct 2, 2018
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.

2 participants