panelprovider: remove panel border radius override

[JonasBa]

This is just a tentative PR and I'm opening it for discussion as I am not a fan of the contextual override of theme values inside PanelProvider

My main motivation for removing this is that I believe it is non intuitive and a cause for confusion. For example, someone might use theme.borderRadius when styling their component and later wonder why its value is 4px at runtime when our theme definition hardcodes it as 6px? If you goto definition, it takes you to the hardcoded value inside our theme, and multiple theme providers is not something that you would expect here. Tracking this down can be tedious and tricky as you can no longer grep the codebase for border-radius: 4px to see which components use it, which makes tracking usage difficult.

Another reason for removal is the question of when to use such a provider? We use it in select control form input, but not components like composite select... should we? Are there components that should be using this today but don't?

On top of that, there is also the question of naming - a PanelProvider inside an Alert or DropdownBubble is not an intuitive API (especially when PanelProvider is more like a border radius provider) and not using it will not scream at you.

I have made the change to remove the provider to try and find the most visually impacted components and gauge the blast radius. The component that stood out most is our new search query builder components (probablyb because of the amount of borders and pills that we render in a narrow space). See before and after below where the radius increases from 4px to 6px.

Before

Removing the provider

Looking a level deeper, the most visually impacted elements here are ListBoxSectionButton, RecentFilterPill and FilterWrapper, which are all components that seem to only be used inside the query builder input, so why shouldn't we just specify a 4px radius on them directly? See commit da22be2 where I added the direct 4px border radius which visually pretty much brings us back to where we started (minus the provider confusion)

There is likely a future where a 4px is defined on the theme directly, and components can use the smaller radius directly, or we can find alternative ways to override such functionality without exposing it to developers.

[codecov]

❌ 1 Tests Failed:

Tests completed	Failed	Passed	Skipped
8464	1	8463	3

View the top 1 failed test(s) by shortest run time

Dashboards > Detail prebuilt dashboards assigns unique IDs to all widgets so grid keys are unique

Stack Traces | 1.41s run time

Error: Unable to find an element with the text: Default Widget 1. This could be because the text is broken up by multiple elements. In this case, you can provide a function for your text matcher to make your matcher more flexible.

Ignored nodes: comments, script, style
...
    at waitForWrapper (.../sentry/sentry/node_modules/@.../dom/dist/wait-for.js:163:27)
    at .../sentry/sentry/node_modules/@.../dom/dist/query-helpers.js:86:33
    at Object.<anonymous> (.../views/dashboards/detail.spec.tsx:195:48)
    at Promise.then.completed (.../sentry/sentry/node_modules/jest-circus/build/utils.js:298:28)
    at new Promise (<anonymous>)
    at callAsyncCircusFn (.../sentry/sentry/node_modules/jest-circus/build/utils.js:231:10)
    at _callCircusTest (.../sentry/sentry/node_modules/jest-circus/build/run.js:316:40)
    at processTicksAndRejections (node:internal/process/task_queues:105:5)
    at _runTest (.../sentry/sentry/node_modules/jest-circus/build/run.js:252:3)
    at _runTestsForDescribeBlock (.../sentry/sentry/node_modules/jest-circus/build/run.js:126:9)
    at _runTestsForDescribeBlock (.../sentry/sentry/node_modules/jest-circus/build/run.js:121:9)
    at _runTestsForDescribeBlock (.../sentry/sentry/node_modules/jest-circus/build/run.js:121:9)
    at run (.../sentry/sentry/node_modules/jest-circus/build/run.js:71:3)
    at runAndTransformResultsToJestFormat (.../sentry/sentry/node_modules/jest-circus/build/legacy-code-todo-rewrite/jestAdapterInit.js:122:21)
    at jestAdapter (.../sentry/sentry/node_modules/jest-circus/build/legacy-code-todo-rewrite/jestAdapter.js:79:19)
    at runTestInternal (.../sentry/sentry/node_modules/jest-runner/build/runTest.js:367:16)
    at runTest (.../sentry/sentry/node_modules/jest-runner/build/runTest.js:444:34)
    at Object.worker (.../sentry/sentry/node_modules/jest-runner/build/testWorker.js:106:12)

To view more test analytics, go to the Test Analytics Dashboard
_{📋 Got 3 mins? Take this short survey to help us improve Test Analytics.}

[ryan953]

The justification for removal makes sense to me, especially since we've only found 3 spots that needed 4px hard-coded to in the 'do it manually' case.

---

These kinds of providers make sense to me. But i think we'd really have to have an ecosystem where we're not writing the raw css at all; so that the consumer side of things was more obvious about using a nested radius. Another example where the same situation could play out the same way is if we had an <H1> tag or a <Title> tag that picked the right section heading level based on nesting. That kind of thing works great only if people stop writing <h1> and friends directly and use the abstraction.

[JonasBa]

These kinds of providers make sense to me. But i think we'd really have to have an ecosystem where we're not writing the raw css at all; so that the consumer side of things was more obvious about using a nested radius. Another example where the same situation could play out the same way is if we had an <H1> tag or a <Title> tag that picked the right section heading level based on nesting. That kind of thing works great only if people stop writing <h1> and friends directly and use the abstraction.

I agree, it'd be great if we approached this more systematically integrate it into an actual component system as opposed to having it directly exposed to developers

[natemoo-re]

I agree with removing this Provider and hard-coding the smaller border-radius for this case.

Automatic border-radius adjustment is definitely a good idea (check out this fascinating ariakit experiment), but this particular implementation is unintuitive and pretty unexpected. While I would normally not suggest removing something in favor of an alternative that hasn't been designed yet, the impact here seems minimal and this change will make the UI2 migration easier, so I'm in favor.

[malwilley]

FYI there are other places that this will affect like the issue stream table and discover tables, or any buttons within an hover overlay. When @vuluongj20 originally added this, the intent was to automatically adjust the buttons within panels so that the corners would look more visually pleasing (a smaller corner radius within an element with a larger corner radius).

I agree it's a bit confusing that I added it in the search query builder - that would definitely be done in a better way. If we still want to adjust the border radius in cases like these I think I'd rather rename PanelProvider since it does work pretty well IMO.

[JonasBa]

FYI there are other places that this will affect like the issue stream table and discover tables, or any buttons within an hover overlay. When @vuluongj20 originally added this, the intent was to automatically adjust the buttons within panels so that the corners would look more visually pleasing (a smaller corner radius within an element with a larger corner radius).

I agree it's a bit confusing that I added it in the search query builder - that would definitely be done in a better way. If we still want to adjust the border radius in cases like these I think I'd rather rename PanelProvider since it does work pretty well IMO.

I do agree that it looks nicer. Afaik we have no actual guidelines about this (maybe @Jesse-Box or someone else from design would know) and we are probably not using this very guideline very consistently either, so the visual aspect is somewhat negated by the inconsistent usage. My opinion is that if we were to have this sort of logic, then it should not be exposed to developers (exported), as we cannot realistically expect them to know when to use.

[Jesse-Box]

The panel theme provider is probably doing what I'm going to describe, but if the developer needs a nested border-radiuses effect ,the math is:

childBorderRadius + parentPadding

For example;
a button with a 6px borderRadius and its parent container has a padding of 6px. If the goal is to apply a nested border radius to the parent, then the right value would be 12px (6px + 6px).

From a design perspective, this isn't always needed, and I'd suggest offering some function for the developer, without it being the default behaviour.