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

Code Insights: Support color for compute powered insight #40038

Merged
merged 8 commits into from Aug 11, 2022

Conversation

vovakulikov
Copy link
Contributor

@vovakulikov vovakulikov commented Aug 5, 2022

Closes #38832
Fixes #39917

Background

Considering a given timeframe for Sourcegraph 4.0, we disable multi-series support for compute-powered insight. This PR

Test plan

  • Go to the compute insight creation UI
  • Create compute insight (make sure that you can't have more than one series)
  • See that this insight was created correctly, and you can see it on the dashboard page.

App preview:

Check out the client app preview documentation to learn more.

@vovakulikov vovakulikov self-assigned this Aug 5, 2022
@cla-bot cla-bot bot added the cla-signed label Aug 5, 2022
Copy link
Contributor

@chwarwick chwarwick left a comment

Choose a reason for hiding this comment

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

This appears good to me. I did notice that it takes on Track Changes edit behavior where you could remove the 1 and only series, and not the capture group behavior where you can't. I think that is ok assuming we intend to eventually add back the ability to have multiple series.

Copy link
Contributor

@AlicjaSuska AlicjaSuska left a comment

Choose a reason for hiding this comment

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

Thank you for making this change @vovakulikov

  1. we need to change the copy on the 'group results' card to mirror this change from

Insight that groups results by repository, path, author or date. You will define each data series manually.

To:

Insight based on a custom Sourcegraph search query that groups results by repository, path, author or date.

  1. Small visual issue: disabled group buttons (author and date) have all round corners. When buttons become active, issue no longer exists:

Screenshot 2022-08-08 at 14 28 09

  1. Not critical, can be out of scope: I'm not sure about how useful is to keep both 'name' for the series and 'name' for the chart. In this case when we only have one series, they would probably be the same in most cases. My proposition is to remove the 'name' field form the data series form for now. Also, data series name is not displayed in the final chart on the dashboard.

  2. Important: when creating insight from the 'all insights' dashboard, it was not saved. I couldn't find it in 'All insights' (loom). When creating an insight from my private dashboard, insight was added there correctly (loom).

@vovakulikov
Copy link
Contributor Author

Thanks @chwarwick @AlicjaSuska for your review

  • About remove behavior, this is a good comment. The compute creation UI reuses the same data series component that we use for the track changes creation UI. I didn't refactor this component because as you mentioned we perhaps will get this behavior for compute insight anyway.

  • copy on the 'group results'. @AlicjaSuska thanks for catching this. I updated the copy.

  • Small visual issue: disabled group buttons. This problem comes from the wildcard component and we've seen this before while were creating this page initially. I have an issue for it here Wildcard: Button group has incorrect visual behaviour  #40094

  • Not critical, can be out of scope: I'm not sure about how useful is to keep both 'name' for the series and 'name' for the chart. This is a good call. I agree that we don't need to have this name field here if we have only one series. Currently this component has been reused from the track changes creation UI and it makes it difficult to just remove it for compute powered creation UI. Let's track this in a separate issue here Code Insights: Remove data series name field for compute powered series #40095. I'm not sure that we will have time for it but let's mark this as a stretch issue for the iteration 26

  • Important: when creating insight from the 'all insights' dashboard, it was not saved. I couldn't find it in 'All insights' I tried to repro this in sourcegraph.sourcegraph staging and couldn't catch this. But this sounds like BE problem. I will try to catch this and notify BE engs if needed, thanks for noticing this

@vovakulikov vovakulikov force-pushed the vk/one-series-compute-insight branch 2 times, most recently from 968e0f9 to 53ccfe9 Compare August 9, 2022 08:43
@vovakulikov
Copy link
Contributor Author

This should be fixed now in this branch

Important: when creating insight from the 'all insights' dashboard, it was not saved. I couldn't find it in 'All insights'

@AlicjaSuska
Copy link
Contributor

Thank you @vovakulikov for making changes and creating the issues. I think everything looks good now, the insight also was saved in 'all insights' correctly.

@AlicjaSuska AlicjaSuska self-requested a review August 10, 2022 15:05
@vovakulikov vovakulikov merged commit bd796dc into main Aug 11, 2022
@vovakulikov vovakulikov deleted the vk/one-series-compute-insight branch August 11, 2022 09:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants