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

ARROW-18391: [R] Fix the version selector dropdown in the dev docs #14800

Merged
merged 2 commits into from
Dec 1, 2022

Conversation

thisisnic
Copy link
Member

@thisisnic thisisnic commented Dec 1, 2022

For context, this overrides the default navbar HTML with a custom version which makes a single change - adding a span called "version" which the JS will override. An identically-named span existed in the template used with Bootstrap 3, which is why no JavaScript changes were needed. This approach is almost identical to that in this PR to the pkgdown package which implements the same thing: r-lib/pkgdown#2072

I built this locally, and it appears to have successfully added the dropdown back in.

image

@github-actions
Copy link

github-actions bot commented Dec 1, 2022

@github-actions
Copy link

github-actions bot commented Dec 1, 2022

⚠️ Ticket has not been started in JIRA, please click 'Start Progress'.

@thisisnic thisisnic marked this pull request as ready for review December 1, 2022 14:49
@thisisnic thisisnic changed the title ARROW-18391: [R] Fix the version selector dropdownAdd navbar template which includes "version" span [WIP] ARROW-18391: [R] Fix the version selector dropdownAdd navbar template which includes "version" span Dec 1, 2022
@thisisnic thisisnic changed the title ARROW-18391: [R] Fix the version selector dropdownAdd navbar template which includes "version" span ARROW-18391: [R] Fix the version selector dropdown Dec 1, 2022
@thisisnic thisisnic marked this pull request as draft December 1, 2022 15:07
@thisisnic thisisnic marked this pull request as ready for review December 1, 2022 15:07
@kou
Copy link
Member

kou commented Dec 1, 2022

Can I confirm what problem is fixed by this?

I confirmed the current https://arrow.apache.org/docs/r/ . The version dropdown shows "10.0.0.9000 (dev)" not "10.0.0 (release)". Is it a problem that is fixed by this PR?

I also confirmed that "9.0.0" in https://arrow.apache.org/docs/r/ can move to https://arrow.apache.org/docs/9.0/r/ . I think that it's expected.
I also confirmed that "10.0.0 (release)" in https://arrow.apache.org/docs/9.0/r/ can move to https://arrow.apache.org/docs/r/ . I think that it's expected too.

@thisisnic
Copy link
Member Author

thisisnic commented Dec 1, 2022

Can I confirm what problem is fixed by this?

I confirmed the current https://arrow.apache.org/docs/r/ . The version dropdown shows "10.0.0.9000 (dev)" not "10.0.0 (release)". Is it a problem that is fixed by this PR?

I also confirmed that "9.0.0" in https://arrow.apache.org/docs/r/ can move to https://arrow.apache.org/docs/9.0/r/ . I think that it's expected. I also confirmed that "10.0.0 (release)" in https://arrow.apache.org/docs/9.0/r/ can move to https://arrow.apache.org/docs/r/ . I think that it's expected too.

The problem is that in a recent PR (since 10.0.1) we upgraded to Bootstrap 5 from Bootstrap 3. In the HTML templates which the pkgdown package (used to generate the docs) uses, there is a span called "version" in the template for Bootstrap 3 but not for Bootstrap 5. The JavaScript we use to implement the version switcher replaces that span with the version switcher. In the dev docs, the version switcher is not working because of this: https://arrow.apache.org/docs/dev/r/

I'll update the title to make that more clear, as it's not apparent, sorry.

@thisisnic thisisnic changed the title ARROW-18391: [R] Fix the version selector dropdown ARROW-18391: [R] Fix the version selector dropdown in the dev docs Dec 1, 2022
Copy link
Member

@kou kou left a comment

Choose a reason for hiding this comment

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

Thanks.
I understand.

I couldn't confirm the change details but +1 because you showed that it works on your local.

@thisisnic thisisnic merged commit f51ea4d into apache:master Dec 1, 2022
@ursabot
Copy link

ursabot commented Dec 2, 2022

Benchmark runs are scheduled for baseline = 7f04d91 and contender = f51ea4d. f51ea4d is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Failed ⬇️0.31% ⬆️0.03%] test-mac-arm
[Failed ⬇️0.3% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.1% ⬆️0.0%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] f51ea4de ec2-t3-xlarge-us-east-2
[Failed] f51ea4de test-mac-arm
[Failed] f51ea4de ursa-i9-9960x
[Finished] f51ea4de ursa-thinkcentre-m75q
[Finished] 7f04d919 ec2-t3-xlarge-us-east-2
[Failed] 7f04d919 test-mac-arm
[Failed] 7f04d919 ursa-i9-9960x
[Finished] 7f04d919 ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

@ursabot
Copy link

ursabot commented Dec 2, 2022

['Python', 'R'] benchmarks have high level of regressions.
ursa-i9-9960x

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

Successfully merging this pull request may close these issues.

3 participants