-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Get actions collection on Preview Container update #7385
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #7385 +/- ##
==========================================
- Coverage 56.00% 55.92% -0.08%
==========================================
Files 662 662
Lines 26301 26302 +1
Branches 2551 2551
==========================================
- Hits 14730 14710 -20
- Misses 10862 10887 +25
+ Partials 709 705 -4
... and 9 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
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.
LGTM, just one suggestion to add a comment. We should also add a quick e2e test
src/ui/preview/PreviewContainer.vue
Outdated
updated() { | ||
this.getActionsCollection(this.view); | ||
}, |
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 is an unconventional change and might raise some eyebrows in the future. We should add a // FIXME:
and a brief note explaining why it's here. We should also reference the issue URL
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.
You know what? This totally escaped my mind. We might be able to just use await nextTick()
in the mounted()
hook before running this.getActionsCollection(this.view);
. That way we're not running that on every update, even though I think it technically only gets run once anyway.
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.
Unfortunately, nextTick didn't bail us out this time so I left the updated lifeycle hook and added a fixme to give more context.
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.
Damn! Ok, sounds good
@michaelrogers just need to run a |
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.
LGTM
Closes #7158
Describe your changes:
Added updated lifecycle hook to fetch actions collection after the Preview Container is updated. This resolves not all menu items being available when viewing in Preview mode.
All Submissions:
Author Checklist
type:
label? Note: this is not necessarily the same as the original issue.Reviewer Checklist