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

Use extendPluggableElement for context menu items #2229

Merged
merged 2 commits into from
Aug 23, 2021

Conversation

garrettjstevens
Copy link
Collaborator

Inspired by #2226, this makes it so that plugins that want to add context menu items to displays now do so using the Core-extendPluggableElement extension point. This removes the need for there to be an autorun that watches for new displays and adds "additionalContextMenuItems" to them. Instead, it extends the display state model itself so all instances of the display already have the additional context menu items.

@garrettjstevens garrettjstevens self-assigned this Aug 16, 2021
@github-actions github-actions bot added the needs label triage Needs a label to show in changelog (breaking, enhancement, bug, documentation, or internal) label Aug 16, 2021
@garrettjstevens
Copy link
Collaborator Author

May be helpful to look at the diff ignoring whitespace.

@codecov
Copy link

codecov bot commented Aug 16, 2021

Codecov Report

Merging #2229 (40bfb3e) into main (5c8734c) will decrease coverage by 0.07%.
The diff coverage is 41.25%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2229      +/-   ##
==========================================
- Coverage   62.89%   62.81%   -0.08%     
==========================================
  Files         484      484              
  Lines       22724    22693      -31     
  Branches     5157     5133      -24     
==========================================
- Hits        14292    14255      -37     
- Misses       8162     8168       +6     
  Partials      270      270              
Impacted Files Coverage Δ
plugins/alignments/src/index.ts 100.00% <ø> (ø)
plugins/dotplot-view/src/index.ts 24.44% <24.56%> (-7.25%) ⬇️
plugins/linear-comparative-view/src/index.tsx 18.71% <77.77%> (-5.36%) ⬇️
...lugins/alignments/src/LinearPileupDisplay/model.ts 63.58% <100.00%> (-0.74%) ⬇️
...ments/src/LinearSNPCoverageDisplay/models/model.ts 66.66% <100.00%> (ø)
...BaseLinearDisplay/components/BaseLinearDisplay.tsx 87.23% <100.00%> (ø)
...aseLinearDisplay/models/BaseLinearDisplayModel.tsx 78.61% <100.00%> (+0.27%) ⬆️
plugins/dotplot-view/src/DotplotView/model.ts 61.30% <0.00%> (-1.20%) ⬇️
packages/core/util/index.ts 79.84% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5c8734c...40bfb3e. Read the comment docs.

@rbuels
Copy link
Contributor

rbuels commented Aug 17, 2021

Looks fine to me as is.

There is opportunity to use extension points more directly for context menu items though, such as each display making a 'MyDisplay-contextMenuItems' extension point and letting other plugins add to that

@cmdcolin
Copy link
Collaborator

I think this is a good change as well. It is breaking since it removes the previously recommend addAdditionalContextMenuItems approach but I'd say we can go for it

@garrettjstevens garrettjstevens 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 Aug 20, 2021
@cmdcolin cmdcolin force-pushed the context_menu_extendPluggableElement branch 2 times, most recently from 4b88afa to 554be88 Compare August 20, 2021 19:35
@cmdcolin
Copy link
Collaborator

Fixed up a merge conflict here, and moved the large dotplot onClick callback to a new function ...should be good to go now though

@cmdcolin cmdcolin force-pushed the context_menu_extendPluggableElement branch from 554be88 to 40bfb3e Compare August 20, 2021 19:39
@rbuels rbuels merged commit 40d9fe7 into main Aug 23, 2021
@rbuels rbuels deleted the context_menu_extendPluggableElement branch August 23, 2021 19:05
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.

3 participants