search

sourcegraph / sourcegraph-public-snapshot

Code AI platform with Code Search & Cody
https://sourcegraph.com
Other
10.1k stars 1.27k forks source link

Insights: Revisit placeholder functionality for `MonacoField` #29438

Open umpox opened 2 years ago

umpox commented 2 years ago

Description

This PR: https://github.com/sourcegraph/sourcegraph/pull/29088 introduced data-placeholder functionality to render a styled placeholder over a MonacoQueryInput. This works well and is something which I think the design team has wanted for a while I believe (https://github.com/sourcegraph/sourcegraph/issues/21465), however it has caused some problems due to log spamming

Problem

The change meant that we now spread any additional props onto the wrapper div around MonacoQueryInput. Due to a large amount of prop drilling into this component, it means that we get a lot of log spam when running the application or tests.

image

Temporary solution

I have added a quick escape-hatch to allow only the data-placeholder prop in this PR: https://github.com/sourcegraph/sourcegraph/pull/29439

We should come up with a better solution to avoid having an insights-only prop on a generic component

Possible solutions

  1. Implement the data-placeholder functionality as part of MonacoQueryInput. Potentially fixes https://github.com/sourcegraph/sourcegraph/issues/21465 and could be useful in other parts of the application.
  2. Implement the data-placeholder functionality in the MonacoField insights component only. Through adding its own wrapper div and updating the styling
  3. Fix the prop drilling problem on MonacoQueryInput. We need to do this but it's likely quite a large piece of work.

Thoughts @sourcegraph/code-insights-frontend? I think the first solution is preferred here.

github-actions[bot] commented 2 years ago

Heads up @joelkw @felixfbecker @vovakulikov @unclejustin - the "team/code-insights" label was applied to this issue.

pdubroy commented 2 years ago

I'm not quite sure I understand what (1) would entail. Is it as easy as moving the CSS or is there more to it?

umpox commented 2 years ago

@pdubroy I think it would be a case of moving the CSS and applying the class conditionally if placeholder exists and query is falsy, like this. We might also want to do some work to improve accessibility, as I don't think the placeholder would be read out by a screen reader in this way

fkling commented 2 years ago

FWIW, I'm currently exploring moving the query input to CodeMirror, which has a placeholder extension, so maybe that would avoid this whole problem.