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

Refactor faceted track selector to use more MST state #4094

Merged
merged 9 commits into from Nov 29, 2023
Merged

Conversation

cmdcolin
Copy link
Collaborator

The current faceted track selector uses mostly react hooks (and quite a few of them, useMemo, useReducer, useState, useEffect, prop drilling the useReducer/useState callbacks to multiple components, etc.) to try to synchronize relatively complex state. This changes it to mobx-state-tree which has a number of simplifying effects IMO

@github-actions github-actions bot added the needs label triage Needs a label to show in changelog (breaking, enhancement, bug, documentation, or internal) label Nov 28, 2023
@cmdcolin cmdcolin added bug Something isn't working and removed needs label triage Needs a label to show in changelog (breaking, enhancement, bug, documentation, or internal) labels Nov 28, 2023
@cmdcolin cmdcolin force-pushed the faceted_observer branch 2 times, most recently from 6e5e013 to 7fdd2e7 Compare November 28, 2023 16:21
Copy link

codecov bot commented Nov 28, 2023

Codecov Report

Attention: 61 lines in your changes are missing coverage. Please review.

Comparison is base (05e1776) 63.24% compared to head (7fdd2e7) 63.35%.
Report is 1 commits behind head on main.

❗ Current head 7fdd2e7 differs from pull request most recent head 69fce71. Consider uploading reports for the commit 69fce71 to get more accurate results

Files Patch % Lines
...rc/HierarchicalTrackSelectorWidget/facetedModel.ts 64.93% 27 Missing ⚠️
...ectorWidget/components/faceted/FacetedSelector.tsx 0.00% 11 Missing ⚠️
...SelectorWidget/components/faceted/FacetFilters.tsx 0.00% 9 Missing ⚠️
...electorWidget/components/faceted/FacetedHeader.tsx 0.00% 8 Missing ⚠️
...kSelectorWidget/components/faceted/FacetFilter.tsx 0.00% 6 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4094      +/-   ##
==========================================
+ Coverage   63.24%   63.35%   +0.10%     
==========================================
  Files        1045     1046       +1     
  Lines       30616    30653      +37     
  Branches     7314     7323       +9     
==========================================
+ Hits        19362    19419      +57     
+ Misses      11084    11062      -22     
- Partials      170      172       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@cmdcolin cmdcolin changed the title Add more mobx-state-tree machinery to faceted track selector Fix issue with metadata.description field colliding with regular track description field in faceted track selector Nov 28, 2023
@cmdcolin cmdcolin marked this pull request as draft November 29, 2023 15:16
@cmdcolin
Copy link
Collaborator Author

I think this PR is still a good approach but there is a small bug where if you hide a column, it then resets the widths for all columns after that. hard to fix properly, so will put this PR in draft, and make a new one...

@cmdcolin
Copy link
Collaborator Author

actually, interestingly, the current main branch has the same bug (it resets the widths after you hide a column) so this is not necessarily a regression on this branch....still would be good to fix but this branch could go forward

@cmdcolin cmdcolin changed the title Fix issue with metadata.description field colliding with regular track description field in faceted track selector Refactor faceted track selector to use more MST state Nov 29, 2023
@cmdcolin cmdcolin marked this pull request as ready for review November 29, 2023 16:07
@cmdcolin cmdcolin merged commit 2571245 into main Nov 29, 2023
11 checks passed
@cmdcolin cmdcolin deleted the faceted_observer branch November 29, 2023 16:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant