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

Show version/language selectors below the title #1601

Merged
merged 31 commits into from
Sep 20, 2024
Merged

Conversation

humitos
Copy link
Member

@humitos humitos commented Sep 17, 2024

Allow users to decide whether or not showing a version and language selectors below the title of the documentation.

I'm adding two new configs here:

  • version_selector
  • language_selector

I'm re-using the code written some time ago in #438 for CSS styling.

Ideally, we should include this in the 3.0.0 final version and release it on October 7th as planned. That way, people using our own theme will get a nice and integrated version/language selector without the need of using flyout_display=attached which re-implements all the flyout and requires disabling the floating one to avoid duplications.

Example

Peek 2024-09-17 16-30

@humitos humitos requested a review from a team as a code owner September 17, 2024 12:46
src/sass/_theme_layout.sass Outdated Show resolved Hide resolved
Copy link
Member

@ericholscher ericholscher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reviewed the Python & JS, which looks pretty solid. Not knowledgable enough on the CSS :)

docs/configuring.rst Show resolved Hide resolved
docs/configuring.rst Outdated Show resolved Hide resolved
sphinx_rtd_theme/layout.html Show resolved Hide resolved
sphinx_rtd_theme/layout.html Outdated Show resolved Hide resolved
sphinx_rtd_theme/static/js/versions.js_t Outdated Show resolved Hide resolved
sphinx_rtd_theme/static/js/versions.js_t Outdated Show resolved Hide resolved
sphinx_rtd_theme/static/js/versions.js_t Outdated Show resolved Hide resolved
sphinx_rtd_theme/static/js/versions.js_t Outdated Show resolved Hide resolved
sphinx_rtd_theme/static/js/versions.js_t Outdated Show resolved Hide resolved
sphinx_rtd_theme/static/js/versions.js_t Outdated Show resolved Hide resolved
src/sass/_theme_layout.sass Outdated Show resolved Hide resolved
sphinx_rtd_theme/static/js/versions.js_t Outdated Show resolved Hide resolved
@humitos
Copy link
Member Author

humitos commented Sep 19, 2024

I've addressed all the feedback received. This is how it looks now:

Peek 2024-09-19 11-59

I think it's good enough to me. Let me know if you have any other feedback.

@ericholscher
Copy link
Member

This looks like a great start. I'd love to have some indication of what the strings are (eg. the branch and language icons next to them), but that can be added in the next iteration.

@agjohnson
Copy link
Collaborator

This looks good. I have a lot of CSS fixes that would help this, it's probably easier to just show the change though.

The width and padding could use tuning, as the field width look unintentional stretched to the nav bar width

image

I wouldn't add icons here yet, I think I'd play around with making this a combination dropdown field instead of native select elements next. Making this into a custom dropdown gives us some room to explain default versions/languages, add icons for versions/prs/languages, and add more supporting context to the UI that is missing with just a select.

@agjohnson
Copy link
Collaborator

@humitos how are you testing this locally?

@humitos
Copy link
Member Author

humitos commented Sep 19, 2024

I'm not 😅. I'm just pushing and re building the test-builds version

@agjohnson
Copy link
Collaborator

agjohnson commented Sep 19, 2024

Roger. What would we need to do to make this locally reproducible? Remote testing seems okay for an initial test, but we don't want to hide these styles from other contributors or things could be prone to breaking.

What would be easiest? Mock the custom event with placeholder data?

@humitos
Copy link
Member Author

humitos commented Sep 19, 2024

Yeah, I didn't want to jump to deep in the mocking for this just yet.

I suppose we can follow the pattern we had for multiple versions on debugging but in this case fire the custom event with mocked data. Shouldn't be hard

@agjohnson
Copy link
Collaborator

agjohnson commented Sep 19, 2024

I threw up my changes at #1603 because of the issues with testing locally. It changes the fields to variable width instead of hardcoding them to be 10em.

We should track fixing the local testing issue, we'll need that for contributors and our own maintenance.

* Tuning on select width and icon placement

* Use better cursor

* Add a max width to selects so that they can't overflow

* Only add after pseudo element if select exists

* Lint
@humitos
Copy link
Member Author

humitos commented Sep 20, 2024

Mock the custom event with placeholder data?

@agjohnson I've done this at #1606

@humitos
Copy link
Member Author

humitos commented Sep 20, 2024

@agjohnson this PR is ready to reveiw and merge 👍🏼

Copy link
Collaborator

@agjohnson agjohnson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good for a test! I'm curious to see what this change does in the wild, I've wanted this for a while.

* Debug: trigger Read the Docs Addons event to test/debug locally

Closes #1605

* Push missing file
@humitos humitos merged commit 4cab02f into master Sep 20, 2024
7 checks passed
@humitos humitos deleted the humitos/selectors branch September 20, 2024 18:01
humitos added a commit that referenced this pull request Oct 1, 2024
This is a regression introduced in
#1601

This PR triggers the search modal when clicking on the "Search docs" input from
- inside the flyout if present
- the top left navbar
humitos added a commit that referenced this pull request Oct 2, 2024
This is a regression introduced in
#1601

This PR triggers the search modal when clicking on the "Search docs" input from
- inside the flyout if present
- the top left navbar
@cristian64
Copy link

With display_version removed, what's the workflow now for showing a single, static version in the navigation bar for docs that are not hosted on Read the Docs?

@humitos
Copy link
Member Author

humitos commented Oct 29, 2024

@cristian64 Hi, another user also asked this question and opened an issue at #1624. Please, subscribe there get updates on this topic. We will be working in a solution for this case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants