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

Set language for tabs in URL query #1202

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

titusfortner
Copy link

@titusfortner titusfortner commented Aug 29, 2022

I've been using this in custom-hacked code for a while, but now that I'm upgrading to the most recent tabpane implementation (nice work on that!), I figured I should submit it to see if I can avoid adding my own hacks. :)

This would allow users to link directly to the language they want:
https://selenium.dev/documentation/webdriver/actions_api/mouse/?language=ruby#forward-click

Update:
New syntax uses tab. (seems easier than the more descriptive activeTab):
https://selenium.dev/documentation/webdriver/actions_api/mouse/?tab=ruby#forward-click

@google-cla
Copy link

google-cla bot commented Aug 29, 2022

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

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@geriom
Copy link
Collaborator

geriom commented Aug 31, 2022

This is not working for me. It seems the persistent tab functionality is broken, but has nothing to do with your PR. I'm seeing an error on console:

image

I'm going to try and figure out what's going on.

The page you linked to also shows an error, but a different one:

image

That one is likely caused by a recent update to the tabpane shortcode that changed handleClcik to persistLang. Updating to the latest docsy release should fix that, but may introduce the new error I am seeing.

@geriom geriom self-requested a review August 31, 2022 17:38
@titusfortner
Copy link
Author

Looks like we're not actually using the latest version of docsy in our code right now either. Trying to update all the things right now. Let me put this in draft, and I'll ping you when I can verify it works. Thanks.

@titusfortner titusfortner marked this pull request as draft August 31, 2022 17:48
@titusfortner titusfortner force-pushed the language_url branch 2 times, most recently from 25807c5 to 0d31df6 Compare September 5, 2022 03:44
@titusfortner titusfortner marked this pull request as ready for review September 5, 2022 03:44
@titusfortner
Copy link
Author

Ok, I got the selenium.dev example working. We're using a version of docsy from a couple months ago, but the only commits that might change things are in #1037, and I don't see anything specific there.

My previous PR commit didn't set the local session properly, but that shouldn't result in the error you are seeing... I'm not a JS dev, though and don't know what $('#' + element.id).tab('show') is actually doing.

Also as a side note, is there a reason for the change from code=true to text=false? One option literally results in a code tag being generated and the other can be either html or markdown depending on the Hugo syntax used. I guess I think "plain text" when I see "text" (and also I don't want to update everything again). But also I know my project isn't the only one that uses git submodules instead of actual releases....

@deining
Copy link
Collaborator

deining commented Sep 5, 2022

This is not working for me. It seems the persistent tab functionality is broken, but has nothing to do with your PR. I'm seeing an error on console:

image

This doesn't work for me either (with tip of main branch, I get the same message on the error console). I started looking into that and found out that this is a regression introduced with 10fae88. However, it's not fully clear to me yet what exactly is going wrong here. I hope this information can serve as starting point for further investigations.

@geriom
Copy link
Collaborator

geriom commented Sep 14, 2022

I tried this on you website and it works as expected, but for some reason it doesn't work when I try it with the docsy user guide. It's probably related to another issue and not your implementation. I gonna take another look today and get it merged.

@titusfortner
Copy link
Author

Well, We're not using the latest version of Docsy, so it's possible something else is getting in the way. :(

I don't have time to dig into it, so we can put this into draft if you want and I can revisit it once @diemol updates us to the latest version of Docsy.

@deining deining self-requested a review September 14, 2022 20:41
Copy link
Collaborator

@deining deining left a comment

Choose a reason for hiding this comment

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

I tried this on you website and it works as expected, but for some reason it doesn't work when I try it with the docsy user guide.

I can't confirm- For me, setting the language via URL parameter works as expected, both with a module site from scratch and with the user guide.

Overall, this PR looks good to me and is a useful addition to the tabpane shortcode.

});
if (params.language !== null) {
activeLanguage = params.language;
localStorage.setItem('active_language', activeLanguage);
Copy link
Collaborator

@deining deining Sep 14, 2022

Choose a reason for hiding this comment

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

I am uncertain if we should persist the active language given when given as URL parameter.
Currently, the language is always persisted, even if the user set persistLang=false for the tab pane. So maybe it is better to not persist the language at all when given as URL parameter.

@geriom
Copy link
Collaborator

geriom commented Sep 15, 2022

I can't confirm- For me, setting the language via URL parameter works as expected, both with a module site from scratch and with the user guide.

Must be something with my local environment, the Netlify preview also works fine.

I am uncertain if we should persist the active language given when given as URL parameter. Currently, the language is always persisted, even if the user set persistLang=false for the tab pane. So maybe it is better to not persist the language at all when given as URL parameter.

I think it's OK to persist the language. I feel that having it in the URL explicitly makes a language selection that supersedes the persistLang flag.

@titusfortner
Copy link
Author

Maybe we can unclear it if they switch to another tab and persistLang isn't set? persistLang just makes much more sense when used in conjunction with #1227.

@chalin chalin modified the milestones: 24Q1, 24Q2 Jan 11, 2024
@chalin chalin modified the milestones: 24Q2, 24Q3 Apr 2, 2024
@chalin chalin modified the milestones: 24Q3, 24Q4 Jul 2, 2024
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.

None yet

4 participants