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

Adds code improvements to GridBookmarkWidget #3938

Merged
merged 11 commits into from
Sep 24, 2023
Merged

Adds code improvements to GridBookmarkWidget #3938

merged 11 commits into from
Sep 24, 2023

Conversation

carolinebridge
Copy link
Contributor

@carolinebridge carolinebridge commented Sep 20, 2023

Leverages work done by @cmdcolin from #3932

Accepts most recommended changes to code styling while fixing bugs and preserving functionality.

Summary of changes:

  • uses Set where applicable to improve lookup performance
  • moves dialogs into lazy loaded components
  • removes list of bookmarks to be deleted
  • removes select styling, replaces with smaller component for code readability
  • clarifies messaging for label editing
  • preserves multi-select for assemblies
  • bookmarksWithValidAssemblies needs to use the JSON "hack" to get around state restrictions with mobx -- if there are better alternatives here, open to suggestion. Without mutating the object like this, there is a crash if you try to share selected bookmarks

@github-actions github-actions bot added the needs label triage Needs a label to show in changelog (breaking, enhancement, bug, documentation, or internal) label Sep 20, 2023
@codecov
Copy link

codecov bot commented Sep 20, 2023

Codecov Report

Merging #3938 (0836213) into main (ce05256) will decrease coverage by 0.34%.
The diff coverage is 31.68%.

@@            Coverage Diff             @@
##             main    #3938      +/-   ##
==========================================
- Coverage   64.04%   63.71%   -0.34%     
==========================================
  Files        1013     1018       +5     
  Lines       30177    30189      +12     
  Branches     7195     7200       +5     
==========================================
- Hits        19327    19235      -92     
- Misses      10687    10790     +103     
- Partials      163      164       +1     
Files Changed Coverage Δ
packages/core/assemblyManager/assemblyManager.ts 74.64% <ø> (ø)
...ookmarkWidget/components/DeleteBookmarksDialog.tsx 0.00% <0.00%> (ø)
...ookmarkWidget/components/ImportBookmarksDialog.tsx 0.00% <0.00%> (ø)
...BookmarkWidget/components/ShareBookmarksDialog.tsx 0.00% <0.00%> (ø)
.../GridBookmarkWidget/components/ImportBookmarks.tsx 60.00% <42.85%> (+27.69%) ⬆️
.../GridBookmarkWidget/components/DeleteBookmarks.tsx 55.55% <50.00%> (+10.10%) ⬆️
...c/GridBookmarkWidget/components/ShareBookmarks.tsx 55.55% <50.00%> (-11.76%) ⬇️
...gins/grid-bookmark/src/GridBookmarkWidget/model.ts 60.46% <61.53%> (-3.49%) ⬇️
...GridBookmarkWidget/components/AssemblySelector.tsx 61.90% <72.72%> (-3.32%) ⬇️
...ookmarkWidget/components/ExportBookmarksDialog.tsx 86.66% <86.66%> (ø)
... and 5 more

... and 5 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Base automatically changed from fixing-bookmarks-bugs to main September 20, 2023 19:13
@carolinebridge carolinebridge added enhancement New feature or request and removed needs label triage Needs a label to show in changelog (breaking, enhancement, bug, documentation, or internal) labels Sep 20, 2023
@carolinebridge carolinebridge changed the title adding changes from grid_mods Adds code improves to GridBookmarkWidget Sep 20, 2023
@carolinebridge carolinebridge changed the title Adds code improves to GridBookmarkWidget Adds code improvements to GridBookmarkWidget Sep 20, 2023
@cmdcolin cmdcolin merged commit 77c58f6 into main Sep 24, 2023
@cmdcolin cmdcolin deleted the add-grid-mods branch September 26, 2023 00:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants