zakodium-oss / react-science

React components and tools to build scientific applications.
https://react-science.pages.dev
MIT License
2 stars 6 forks source link

Rewrite buttons to be wrappers around blueprintjs's buttons #571

Closed stropitek closed 10 months ago

stropitek commented 11 months ago

https://blueprintjs.com/docs/#core/components/buttons

Here I think we can add a bit of sugar on top of the blueprint API.

For the Button component:

We can make it easier to add a tooltip to a Button than would be for any other component, because it's a frequent use case.

But mostly the component should just forward props to the blueprintjs component.

API suggestion

type ButtonProps = BlueprintButtonProps & {
  tooltipProps: Omit<BlueprintTooltipProps, 'children'> 
}
/>

There is a known limitation when using a tooltip on a Button, it will not work if the button is disabled. Therefore I suggest that we render a blueprintjs AnchorButton when disabled, so that the tooltip still shows up.

wadjih-bencheikh18 commented 11 months ago

I don't like that in BlueprintButtonProps there is two props large and small in order to change button size. And also It's unstable if we toggle them randomly. I suggest to use one prop size: 'regular' | 'small' | 'large'

stropitek commented 11 months ago

I suggest to use one prop size: 'regular' | 'small' | 'large'

I agree in general, but I think the goal of this component is to add something on top of the base bluprintjs button, not change / improve its api.

When building applications with react-science, we will import re-usable components with which are either:

I think consistency wins over usability

And also It's unstable if we toggle them randomly

What do you mean? It produces strange results when you activate both props together?

wadjih-bencheikh18 commented 11 months ago

It produces strange results when you activate both props together?

Yes

wadjih-bencheikh18 commented 11 months ago

Therefore I suggest that we render a blueprintjs AnchorButton when disabled, so that the tooltip still shows up.

There's a diffrance between BlueprintButtonProps and BlueprintAnchorButtonProps types. Do you suggest a solution in order to keep possible props to our component.

stropitek commented 11 months ago

AFAIK the difference in the props only come down to the differences between the attributes a <button> and an <a> element can take. Maybe you can use the intersection between button and a attributes and only allow those?

If that does not work you can only spread props when the button is rendered but for the anchor pass hand-picked props?