-
Notifications
You must be signed in to change notification settings - Fork 36
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
DOP-3424: "On this page" subnav incorrectly displays headings for all language tabs #1262
Conversation
✅ Deploy Preview for mongodb-snooty ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is awesome to see working!
src/components/Tabs/index.js
Outdated
if (!compare(Object.values(activeTabs), activeSelectorIds)) { | ||
setActiveSelectorIds({ | ||
...activeSelectorIds, | ||
tab: Object.values(activeTabs), | ||
}); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This if statement results in a similar local dev warning as the one in MethodSelector
. Is there anything we can do to avoid this? Could we limit this to only be called whenever the tab changes on initial load or on click?
Warning: Cannot update a component (
ContentsProvider
) while rendering a different component (Tabs
). To locate the bad setState() call insideTabs
, follow the stack trace as described in https://reactjs.org/link/setstate-in-render
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For both this and the method selector, when I try to move the statements into useEffect
calls I run into an infinite loop 😭 reading through documentation it seems as if it would be ideal to not put the setState call in useEffect, and this was the solution I came up with, although I missed this warning so thanks for pointing it out... however, I am at my wits' end for getting this logic to work without any issues </3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this can be done in the child context using the parent context, or here within the component, both with similar early returns as above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, I agree with that docs page that chaining setState calls via useEffect
wouldn't be ideal. I think we'd be circumventing any chaining though if we ensure that it's called (1) only once at initial load/render whenever we grab local storage data (we can leverage the existing one, if possible), and (2) whenever a tab is selected.
src/components/Tabs/index.js
Outdated
@@ -102,9 +105,13 @@ const productLandingTabContentStyling = css` | |||
} | |||
`; | |||
|
|||
const activeMethodIncludesActiveTab = (activeTabValues, activeTabsSelector) => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this function name may be misleading since the active tab doesn't necessarily need to be within a method - could be tabs on their own, nested tabs, etc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh good catch. would it be better as activeTabsAncestorIsActive
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
more something like updateActiveTabs
since this is checking the tabs themselves, not necessarily their ancestor (might want to rename activeTabsSelector
to activeSelectors
or something like that)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but this returns a boolean, does not do any updating
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
true! hmmmm... shouldUpdateActiveTabs
? just want it to be clear that it's focusing on the current tabs selected (which may or may not include ancestor tabs)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i've actually removed this check! we can enforce this correction in the tab context, and only if there are active tabs. and this should be enforced every time an active tab is changed. PTAL at the latest commit
Updated the branch including changes mentioned above, and a staging link is provided below to test the mix of method selectors and tabs: Verify driver tabs and nested tabs and procedures here (switch to/from |
Stories/Links:
DOP-3424
This PR extends work done for DOP-4967 (see parser PR here). In the parser, we create a
selector_ids
object that reflects the AST in terms of the order of tabs/method selectors. This object looks something like this:Within the ContentsContext as well as Tabs and Method Selector, we maintain an object that reflects the active selected method options and tabs/tab selectors. This object relies on the Method Selector being a top-level component, and tabs on the same page not having the same tab ID (unless they are sibling tabs that move together). These are continuously compared to ensure that the Contents reflect the selected Tab/Method Selector. This can be extended in the future to include other components as well.
We also store the value of the active tab in local storage on initial load. Previously, this was only reflected for tabs selectors.
To test, please go through the staging links and ensure that 1. the correct headings show up and 2. you can click on them and they direct to the right section of the page.
Current Behavior:
AWS Lambda Page
Staging Links:
Tabs within Method Selector
AWS Lambda (Tabs Selectors)
Drivers Connection (should be unchanged)
Nested Tabs
Notes:
README updates