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

Add disabled prop to Toolbar.Item #614

Closed hamed-musallam closed 9 months ago

hamed-musallam commented 9 months ago

In our current implementation, we've encountered scenarios where we disabled only specific toolbar items. Currently, each toolbar controls multiple items, leading to a need for a dedicated toolbar to control each item individually.

To address this, I propose changing this behavior. The disable option should be implemented at the item level rather than being applied at the toolbar level.

https://github.com/zakodium-oss/react-science/blob/727513420c99793357c58d7ed4f2090c5bf8ddcc/src/components/toolbar/Toolbar.tsx#L123

stropitek commented 9 months ago

We can have both Toolbar sets the default value for all items, but those can be overridden with the item props

hamed-musallam commented 9 months ago

Ok, i could push a PR

hamed-musallam commented 9 months ago

overridden

but it is not the case in intent option, i think this condition should be changed to intent = intentItem ?? toolbarIntent

https://github.com/zakodium-oss/react-science/blob/727513420c99793357c58d7ed4f2090c5bf8ddcc/src/components/toolbar/Toolbar.tsx#L119

stropitek commented 9 months ago

but it is not the case in intent option, i think this condition should be changed to intent = intentItem ?? toolbarIntent

What is not the case?

hamed-musallam commented 9 months ago

but it is not the case in intent option, i think this condition should be changed to intent = intentItem ?? toolbarIntent

What is not the case?

I apologize, I confirmed that the intent value is consistently a literal value and cannot be empty. Therefore, this approach will function correctly in this scenario. However, the same logic operator will not work for disabled options because the user could pass false for the item and the toolbar is true

hamed-musallam commented 9 months ago

What are your thoughts on the size of the toolbar? Do you believe it should be smaller? i mean if we use it as a panel toolbar

https://github.com/zakodium-oss/react-science/assets/35760236/d3c7f0bb-d5fc-4c68-944d-809b0b2ba08a

stropitek commented 9 months ago

It seems ok. But I do think it would make sense to make the toolbar in panels more compact than the main toolbars.