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.

Sign up for GitHub

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

Jump to bottom

Fix the number of available revisions for multi view plots #2836

Merged
merged 6 commits into from
Nov 26, 2022
Merged

Fix the number of available revisions for multi view plots #2836

merged 6 commits into from
Nov 26, 2022

Conversation

sroy3
Copy link
Contributor

@sroy3 sroy3 commented Nov 25, 2022 •
edited
Loading

Fixes #2834

Before:
Screenshot 2022-11-25 at 9 41 59 AM

Now:
Screenshot 2022-11-25 at 11 04 54 AM

@sroy3 sroy3 added the bug Something isn't working label Nov 25, 2022
@sroy3 sroy3 self-assigned this Nov 25, 2022
@sroy3
Copy link
Contributor Author

sroy3 commented Nov 25, 2022

I think the failing test should send the correct data to the webview for flexible plots is simply not fully set up and that the multiSourceEncoding is missing. I'm looking into adding it, but I'm not quite sure how to. @mattseddon if you know how to do this quickly, help would be appreciated.

@mattseddon
Copy link
Contributor

I think the failing test should send the correct data to the webview for flexible plots is simply not fully set up and that the multiSourceEncoding is missing. I'm looking into adding it, but I'm not quite sure how to. @mattseddon if you know how to do this quickly, help would be appreciated.

I'll take a look.

mattseddon
mattseddon reviewed Nov 26, 2022
View reviewed changes
extension/src/test/suite/plots/index.test.ts
{ label: 'workspace' },
{ label: 'main' }
] as SelectedExperimentWithColor[])

stub(Experiments.prototype, 'getBranchRevisions').returns([])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[F] Multi-source test fixture doesn't match the default exp show fixture. The change required would blow this PR out so I have just patched it for now. Will follow-up once this is merged.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@mattseddon mattseddon mentioned this pull request Nov 26, 2022
Merged
@mattseddon mattseddon marked this pull request as ready for review November 26, 2022 23:42
@mattseddon mattseddon requested a review from julieg18 as a code owner November 26, 2022 23:42
@codeclimate
Copy link

codeclimate bot commented Nov 26, 2022

Code Climate has analyzed commit a6270dc and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 100.0% (85% is the threshold).

This pull request will bring the total coverage in the repository to 96.6% (0.0% change).

View more on Code Climate.

@mattseddon mattseddon merged commit 43c4d12 into main Nov 26, 2022
@mattseddon mattseddon deleted the fix-available-revisions branch November 26, 2022 23:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattseddon mattseddon mattseddon approved these changes

@julieg18 julieg18 Awaiting requested review from julieg18

Assignees

@sroy3 sroy3

Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Multi view plots do not always show all revisions
2 participants
@sroy3 @mattseddon