#8076: add starter mod sidebar panels to closedtabs on install #8083
Conversation
…8076-slice-1-add-starter-mod-sidebar-panels-to-closedtabs-on-install
…8076-slice-1-add-starter-mod-sidebar-panels-to-closedtabs-on-install
…8076-slice-1-add-starter-mod-sidebar-panels-to-closedtabs-on-install
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #8083 +/- ##
==========================================
- Coverage 73.33% 73.21% -0.13%
==========================================
Files 1309 1312 +3
Lines 40699 40773 +74
Branches 7566 7569 +3
==========================================
+ Hits 29848 29852 +4
- Misses 10851 10921 +70 ☔ View full report in Codecov by Sentry. |
twschiller
added
enhancement
…to-closedtabs-on-install
…to-closedtabs-on-install
| * @param activatedModComponents the activated mod components | ||
| * @param modComponentDefinitions the mod component definitions | ||
| */ | ||
| export function getModComponentIdsForModComponentDefinitions( |
There was a problem hiding this comment.
I think we should use a different prefix here rather than get* since this isn't "fetching" anything. Generally I think we use select* in cases like this where it's just unpacking something from the inputs and returning a different data structure?
There was a problem hiding this comment.
All of the other functions in this file were prefixed with get, so I was maintaining consistency. I wouldn't think that get would necessarily signify fetching and I'm used to select being used for redux selectors.
| @@ -56,6 +57,10 @@ function eventKeyForEntry(entry: Nullishable<SidebarEntry>): string | null { | |||
|
|
|||
| export { eventKeyForEntry }; | |||
|
|
|||
| export function getEventKeyForPanel(modComponentId: UUID): string { | |||
There was a problem hiding this comment.
Again, not sure we should use get* here since it's more just "calculating" something, not fetching. What about just eventKeyForPanel()?
|
|
||
| // The subset of the sidebar state that we persist | ||
| // See persistSidebarConfig.whitelist | ||
| type SidebarPeristedState = Pick<SidebarState, "closedTabs">; |
There was a problem hiding this comment.
Technically I think this should be Pick<SidebarState, "closedTabs"> & PersistedState. I think the code here is fine, since you are spreading the whole state back onto the result/return, but I just wouldn't want someone to accidentally trim off the _persist field and mess up the redux-persist migrations somehow.
|
When the PR is merged, the first loom link found on this PR will be posted to |
Co-authored-by: Ben Loe <below413@gmail.com>
…to-closedtabs-on-install
grahamlangford
deleted the
8076-slice-1-add-starter-mod-sidebar-panels-to-closedtabs-on-install
branch
What does this PR do?
Reviewer Tips
@/sidebarto@/store/sidebarbecause they're needed in@/backgroundDiscussion
Demo
https://www.loom.com/share/ea9510086b1346808ee289e5a8419d2f
Remaining Work
Checklist