-
Notifications
You must be signed in to change notification settings - Fork 165
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
feat: Allow tab content to be retained in memory #3233
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3233 +/- ##
==========================================
- Coverage 96.44% 96.42% -0.02%
==========================================
Files 791 791
Lines 22568 22586 +18
Branches 7385 7735 +350
==========================================
+ Hits 21765 21778 +13
+ Misses 796 754 -42
- Partials 7 54 +47 ☔ View full report in Codecov by Sentry. |
deb4192
to
36f2647
Compare
src/tabs/index.tsx
Outdated
@@ -60,6 +72,18 @@ export default function Tabs({ | |||
changeHandler: 'onChange', | |||
}); | |||
|
|||
const [viewedTabs, setViewedTabs] = useState(new Set(activeTabId)); | |||
|
|||
useEffect(() => { |
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 effect can be removed in favour of decorating the setActiveTabId method, e.g.
const setActiveTabIdDecorated = (newActiveTabId) => {
setActiveTabId(newActiveTabId);
setViewedTabs(prev => new Set(prev).add(newActiveTabId));
}
This way the related state is set within one render cycle so should be better for performance and is better clear, too.
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.
Given that this is a controllable component, I think we need to use an effect. Although it's unlikely, the active tab could change without setActiveTabId
being called
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, I see. Can the visited tabs be a ref then, e.g.:
const visitedTabsRef = useRef(new Set<string>());
visitedTabsRef.current.add(activeTabId);
Since changing the tab always causes a new render no matter if done in controlled or uncontrolled way, I think this approach should work, too.
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.
good point: there's no explicit re-render required here, and the updated value isn't needed until later (switching away from the tab) anyway
Description
Allow inactive tab content to be rendered into the DOM, for use-cases such as:
Related links, issue #, if available: AWSUI-25984
How has this been tested?
New unit and integ tests
Review checklist
The following items are to be evaluated by the author(s) and the reviewer(s).
Correctness
CONTRIBUTING.md
.CONTRIBUTING.md
.Security
checkSafeUrl
function.Testing
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.