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 tabbed widget JS and CSS to build #2180

Merged
merged 9 commits into from
Aug 24, 2021

Conversation

bmorelli25
Copy link
Member

@bmorelli25 bmorelli25 commented Aug 16, 2021

Summary

We're now using the tabbed widget in at least 10 different documentation books! This PR adds the tabbed widget JS and CSS to the documentation build. I'm confident I've added the CSS correctly, but the JavaScript is another story. It works 😮, but I'm not sure if there's a better way to go about this. @gtback, if this is comically wrong, just let me know and I'll close this PR 😬

How to test this

  1. Checkout this PR: gh pr checkout 2180
  2. Add a simple tabbed widget to any documentation page. Here's a boilerplate widget:
++++
<div class="tabs" data-tab-group="config-agent">
  <div role="tablist" aria-label="Configure agents">
    <button role="tab"
            aria-selected="true"
            aria-controls="cloud-tab-config-agent"
            id="cloud-config-agent">
      Central configuration
    </button>
    <button role="tab"
            aria-selected="false"
            aria-controls="self-managed-tab-config-agent"
            id="self-managed-config-agent"
            tabindex="-1">
      Agent configuration
    </button>
  </div>
  <div tabindex="0"
       role="tabpanel"
       id="cloud-tab-config-agent"
       aria-labelledby="cloud-config-agent">
++++

This is the content for tab #1

++++
  </div>
  <div tabindex="0"
       role="tabpanel"
       id="self-managed-tab-config-agent"
       aria-labelledby="self-managed-config-agent"
       hidden="">
++++

This is the content for tab #2

++++
  </div>
</div>
++++
  1. Build that documentation book and ensure the widget works as expected

Related issues

Closes #1895.

@bmorelli25 bmorelli25 requested a review from gtback August 16, 2021 23:15
@bmorelli25 bmorelli25 self-assigned this Aug 16, 2021
Copy link
Member

@gtback gtback left a comment

Choose a reason for hiding this comment

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

I think we'll want to change the JS so it gets included in /guide/static/docs.js (and thus can be loaded once and cached) rather than inlining it on every page (increasing page sizes and slowing down page loads, even if only marginally), even pages that don't use the widgets.

I'll have to refresh my memory on how that all gets built, but if you want to try to figure it out, great!

@bmorelli25
Copy link
Member Author

Moved JS to resources/web/lib/prettify/prettify.js. That might be right. When running the doc build locally, it looks like JS isn't minified. I'm able to see the new JS when I navigate to http://localhost:8000/guide/static/docs.js, and the JS is applied to the tabbed widgets. But there's a new problem -- this breaks the CSS for some reason. Looking into that now.

@bmorelli25 bmorelli25 marked this pull request as draft August 17, 2021 15:27
@bmorelli25 bmorelli25 marked this pull request as ready for review August 17, 2021 18:10
@bmorelli25
Copy link
Member Author

OK, I think this is ready for review again. I can see the JS at http://localhost:8000/guide/static/docs.js, and the CSS at http://localhost:8000/guide/static/styles.css

@gtback gtback self-requested a review August 17, 2021 19:28
resources/web/lib/prettify/prettify.js Outdated Show resolved Hide resolved
@gtback
Copy link
Member

gtback commented Aug 19, 2021

It seems like the test is failing because it's trying to set the active tab on a test page that doesn't have the tab widget?

Copy link
Member

@gtback gtback left a comment

Choose a reason for hiding this comment

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

This is looking good! I think we just need to update the functions to not do anything if there are no tab widgets on the page.

@bmorelli25
Copy link
Member Author

bmorelli25 commented Aug 19, 2021

I think we just need to update the functions to not do anything if there are no tab widgets on the page.

Interesting! OK, let me try to figure out how to do that 🤔

Relevant failure:

12:17:20 FAIL tests/docs.test.js
12:17:20   ● Test suite failed to run
12:17:20 
12:17:20     TypeError: Cannot read property 'parentNode' of undefined
12:17:20 
12:17:20       21004 | 
12:17:20       21005 | const setActiveTab = target => {
12:17:20     > 21006 |   const parent = target.parentNode;
12:17:20             |                         ^
12:17:20       21007 |   const grandparent = parent.parentNode; // console.log(grandparent);
12:17:20       21008 |   // Remove all current selected tabs
12:17:20       21009 | 
12:17:20 
12:17:20       at parentNode (resources/web/tests/docs.test.js:21006:25)
12:17:20       at Object.<anonymous>.parcelRequire.S3PC../components/alternative_switcher (resources/web/tests/docs.test.js:23818:1)
12:17:20       at call (resources/web/tests/docs.test.js:47:24)
12:17:20       at newRequire (resources/web/tests/docs.test.js:53:14)
12:17:20       at Object.require (resources/web/tests/docs.test.js:23827:14)
12:17:20       at call (resources/web/tests/docs.test.js:47:24)
12:17:20       at newRequire (resources/web/tests/docs.test.js:81:7)
12:17:20       at Object.<anonymous> (resources/web/tests/docs.test.js:8:17)
12:17:20 
12:17:20 Test Suites: 1 failed, 11 passed, 12 total

@bmorelli25
Copy link
Member Author

bmorelli25 commented Aug 20, 2021

I have an idea of how to fix this. Is there a way to run self-test locally so I don't have to push and wait for it to run?

TypeError: Cannot read property 'parentNode' of undefined

I think this is failing because the setActiveTab function requires a target node parameter. When the page is built, and someone clicks on a tab widget, it receives one. But when the tests run, which invokes the setActiveTab function here, no target parameter is passed in.
As it turns out, the setActiveTab and changeTabs functions don't need to be exported and invoked in index.js. I think this might fix the problem as these functions will no longer run automatically when resources/web/tests/docs.test.js runs. See 9b56afd.

@gtback
Copy link
Member

gtback commented Aug 20, 2021

Is there a way to run self-test locally so I don't have to push and wait for it to run?

@bmorelli25 You can change into the resources/web directory and run make check to run just these tests. You can also run make check from the root of the repo to run the full test suite (which is pretty slow because of the integration tests) or make unit_test for just the "fast" tests.

@gtback
Copy link
Member

gtback commented Aug 20, 2021

Once you get the tests passing, feel free to merge!

@bmorelli25
Copy link
Member Author

Will do! It looks like that fixed self-test 🎉 . I'm going to add some docs to the readme before merging. Thanks for all of your help!

@bmorelli25 bmorelli25 merged commit d8acfc4 into elastic:master Aug 24, 2021
@bmorelli25 bmorelli25 deleted the add-tabbed-widget-code-to-build branch August 24, 2021 18:53
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.

Add CSS and Styles from linked-tabs widget
2 participants