search

openedx / public-engineering

General public issue repository for the Open edX engineering community
4 stars 2 forks source link

[DEPR]: edx-sphinx-theme #200

Closed feanil closed 1 year ago

feanil commented 1 year ago

Proposal Date

29 November 2022

Target Ticket Acceptance Date

13 December 2022

Earliest Open edX Named Release Without This Functionality

Palm - 2023-04

Rationale

Removal

This repository https://github.com/openedx/edx-sphinx-theme/ will be archived after doing one more PyPI release which will indicate that the project is deprecated and no longer maintained.

Replacement

Anyone building docs should use the sphinx-book-theme instead.

Deprecation

Migration

People can switch to the sphinx-book-theme by adding the sphinx-book-theme to their requirements and updating their sphinx conf.py to use the sphinx-book-theme.

Additional Info

Discourse Post: https://discuss.openedx.org/t/deprecation-removal-edx-sphinx-theme/8817

robrap commented 1 year ago

Thanks @feanil. I think this is great. I wonder if we could crowdsource a doc for migrating (unless you already have that). For example, are there settings that no longer apply, or should be renamed, or new ones that are useful to know about, etc.

feanil commented 1 year ago

That's a great idea Robert, I think we can collect any items we want to cover on this ticket and then turn it into a "How to Migrate from edx-sphinx-theme to sphinx-book-theme" doc, what do you think?

robrap commented 1 year ago

Also, for additional "Rationale", the old theme had a bunch of styling bugs, including being broken on mobile. See "Fix Sphinx theme" in https://openedx.atlassian.net/wiki/spaces/AC/pages/2624553266/Developer+Docs+Framework+Notes, if you need more details.

jmbowman commented 1 year ago

I agree this is a good idea, just want to make sure we've covered two of the reasons for edx-sphinx-theme's existence that weren't mentioned in the description:

robrap commented 1 year ago

@jmbowman: It’s good to have that background:

  1. I say we review the replacement for a11y and help fix it if that is needed.
  2. For most of the time the Google Doc existed, it was a black hole that collected comments that no one ever looked at. Instead, we could have a how-to on all the ways someone could provide feedback.
robrap commented 1 year ago

I’d clarify that maintenance of a custom theme either adds maintenance cost, or leads to a badly broken theme if it is neglected, which was our case.

feanil commented 1 year ago

@jmbowman good context on the a11y work, I didn't know about that. I've added https://github.com/openedx/docs.openedx.org/issues/224 to review the a11y of the new theme and we can update/make improvements based on that but a quick tab-through and keyboard navigation test seems to show no major issues for now. If we do find some in a more thorough test I think it makes sense to fix forward upstream.

feanil commented 1 year ago

As for the feedback form, there is a new feedback form, there is a new feedback form that Jenna and I are monitoring and it's on all the new pages: image

feanil commented 1 year ago

Given that there were no concerns on the discourse post and all concerns on this issue have been addressed. This deprecation is now accepted.

feanil commented 1 year ago

I've added a bunch of sub-tasks that need to be completed before this theme can be considered deprecated.

MaxFrank13 commented 1 year ago

I've noticed that the update to sphinx 6.0.0 has a breaking change involving the use of jquery see the changelogs. We use jquery in the edx-sphinx-theme repo here.

I believe the version will need to be pinned at 5.3.0 until the deprecation is complete.

robrap commented 1 year ago

Something is odd about the current state of the requirements. Sphinx is supposedly constrained to 1.8.5 here in edx-sphinx-theme. Typically we include constraints for base.in requirements in the package, but that doesn't seem to be happening here, since edx-platform is at 5.3.0 for sphinx.

I wonder if our code to publish constraints is missing this due to a difference in capitalization in base.in (Sphinx vs sphinx), or some other reason?

jmbowman commented 1 year ago

No new edx-sphinx-theme versions have been published since July 1, 2021. That predates our fix to include constraints.txt information in the package metadata, which was made on November 9, 2021.

robrap commented 1 year ago

Note: @jmbowman created https://github.com/openedx/edx-sphinx-theme/issues/197 to handle this constraint issue. This should happen before the repo is archived.

robrap commented 1 year ago

@feanil: Did we ever end up with a how-to for this (see earlier comment)? I noticed @xitij2000 is implementing much of this work, and it would be great for reviewers if decisions were captured in a doc.

For example, I noticed the following changes:

- copyright = edx_theme.COPYRIGHT  # pylint: disable=redefined-builtin
- author = edx_theme.AUTHOR
+ copyright = f'{datetime.now().year}, edX Inc.'  # pylint: disable=redefined-builtin
+ author = 'edX Inc.'

I'm guessing we just went with the old settings, but is edX Inc. what we actually want here?

xitij2000 commented 1 year ago

@robrap The how-to can be found here: https://docs.openedx.org/en/latest/developers/how-tos/switch-to-the-sphinx-book-theme.html

As for the copyright etc. That is a good point. I copied over the values from edx-sphinx-theme, however as you pointed out perhaps they should be updated to say "The Axim Collaborative"

@feanil I know you've updated the value in html_theme_options. Should I update these values for copyright and author for all open PRs?

feanil commented 1 year ago

@xitij2000 yea, that's a good catch, let's do that. The Author should be Axim Collabortive and the same for the Creative-Commons licensensing section.

robrap commented 1 year ago

Thanks @xitij2000 and @feanil.

robrap commented 1 year ago

@xitij2000: I'd propose adding a link to this DEPR and to the how-to in your PR and commit comments. Merged PRs could potentially get the PR description updated as well for context, but I'll leave that to you all to decide.

xitij2000 commented 1 year ago

@xitij2000: I'd propose adding a link to this DEPR and to the how-to in your PR and commit comments. Merged PRs could potentially get the PR description updated as well for context, but I'll leave that to you all to decide.

Will do.

xitij2000 commented 1 year ago

@xitij2000: I'd propose adding a link to this DEPR and to the how-to in your PR and commit comments. Merged PRs could potentially get the PR description updated as well for context, but I'll leave that to you all to decide.

Will do.

I've updated the PRs to include the changes to copyright, author, the commit message, and PR description.

xitij2000 commented 1 year ago

@feanil I think there are just two PRs left here:

Could you have a look?

feanil commented 1 year ago

@xitij2000 reviewed and merged. A quick grep shows the following repos still left, can you take a look at them?

xitij2000 commented 1 year ago

@feanil All these repos seem to have it as a dependency, but don't actually use the theme. I guess that is why they were missing from the initial investigation.

What should be the approach here? Just remove the dependency or add in all the config for these themes?

xitij2000 commented 1 year ago

@xitij2000 reviewed and merged. A quick grep shows the following repos still left, can you take a look at them?

* pytest-warnings-report

* codejail-includes

* edx-repo-health

* pytest-repo-health

* openedx-filters

* repo-tools-data-schema

* opaque-keys

I've created PRs for these repos, but will only be able to test and review it internally next sprint.

feanil commented 1 year ago

Yea, I think we should migrate them to the correct theme anyway and remove the old one. Sounds good about not making more progress till next sprint. Let me know when that starts so I don't bother you before then :-)

xitij2000 commented 1 year ago

Yea, I think we should migrate them to the correct theme anyway and remove the old one. Sounds good about not making more progress till next sprint. Let me know when that starts so I don't bother you before then :-)

I've created PRs for the final batch and they have been reviewed.

feanil commented 1 year ago

@xitij2000 I think we have 3 repos left, you've got PRs in progress for credentials and blockstore it looks like, I just want to make sure you have edx-platform on your radar as well? Also, FYI, I'll be on vacation next week so I'll not be responding to reviews then. I'll be back on June 12th.

xitij2000 commented 1 year ago

@xitij2000 I think we have 3 repos left, you've got PRs in progress for credentials and blockstore it looks like, I just want to make sure you have edx-platform on your radar as well? Also, FYI, I'll be on vacation next week so I'll not be responding to reviews then. I'll be back on June 12th.

Yes, it is on my radar, just pushed that PR as well.

robrap commented 1 year ago

I noticed the old theme when going to https://edx.readthedocs.io/projects/edx-toggles/en/latest/how_to/documenting_new_feature_toggles.html. I guess we don't have a step to ensure that changes have actually made it out to readthedocs, or was this repo missed?

feanil commented 1 year ago

The repo was not missed it looks like: https://github.com/openedx/edx-toggles/pull/261

So likely an issue with the publishing of that specific repo, looks like it's failing to build due to requirements issues with the RTD setup: https://readthedocs.org/projects/edx-toggles/builds/20898194/

robrap commented 1 year ago

Thanks @feanil. I put up https://github.com/openedx/edx-toggles/pull/278 to fix the build.

feanil commented 1 year ago

The edx-sphinx theme has been removed from all openedx repositories. The last change left is to update common-constraints(https://github.com/openedx/edx-lint/pull/349) and then we can close this ticket and archive this repository.