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 Toolbar.PopoverItem #635

Closed moonayyur closed 7 months ago

moonayyur commented 7 months ago

closes #612

cloudflare-pages[bot] commented 7 months ago

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 57bb079
Status: ✅  Deploy successful!
Preview URL: https://6b9f3f8a.react-science.pages.dev
Branch Preview URL: https://612-add-toolbarmenuitem-or-s.react-science.pages.dev

View logs

stropitek commented 7 months ago

Can you make a dedicated story with toolbar popover items? The feature is hard to discover in the stories.

Also if you have ideas on how we can make a popover item distinguishable from a regular item, that would be welcome. For example in photoshop there is this little triangular shape in the corner of the button:

CleanShot 2024-01-25 at 10 52 27

codecov-commenter commented 7 months ago

Codecov Report

Attention: 181 lines in your changes are missing coverage. Please review.

Comparison is base (bf1bfde) 24.26% compared to head (b57c6b9) 23.39%. Report is 4 commits behind head on main.

Files Patch % Lines
stories/components/toolbar.stories.tsx 0.00% 143 Missing and 1 partial :warning:
src/components/toolbar/Toolbar.tsx 28.84% 37 Missing :warning:

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #635 +/- ## ========================================== - Coverage 24.26% 23.39% -0.87% ========================================== Files 223 221 -2 Lines 12903 12778 -125 Branches 234 234 ========================================== - Hits 3131 2990 -141 - Misses 9684 9700 +16 Partials 88 88 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

moonayyur commented 7 months ago

@stropitek I added a right icon to the popover items like in the dropdown component. The icon has different directions when the toolbar is vertical or horizontal

moonayyur commented 7 months ago

@stropitek There is a bug with the vertical control in the Toolbar : https://react-science.pages.dev/stories/?path=/story/components-toolbar--control

By default vertical has the value 'false' and the toolbar is horizontal, when we toggle the button the toolbar becomes vertical, but when we click on it again it doesn't go back to horizontal

stropitek commented 7 months ago

Can you also make a separate story where we mix dropdown items and non-dropdown items? Try to put them at the beginning, at the end and in between.

There are alignment issues as well in the vertical version of the toolbar with dropdown items.

CleanShot 2024-01-26 at 10 38 17

By default vertical has the value 'false' and the toolbar is horizontal, when we toggle the button the toolbar becomes vertical, but when we click on it again it doesn't go back to horizontal

Do you need help with debugging this?

moonayyur commented 7 months ago

Do you need help with debugging this?

It is due to this code in the toolbar width recomputing :

if (!vertical) {
      return;
    }

Removing it fixes the problem

stropitek commented 7 months ago

ping @moonayyur @hamed-musallam

stropitek commented 7 months ago

I'm merging this, but I think there is too much layout difference between the dropdown and the non-dropdown item. The addition of the icon on the right doubles the size of the component.

I'm creating a new issue for that.

hamed-musallam commented 7 months ago

@stropitek

I believe it's essential to maintain consistent layout, width, and height for both dropdown and non-dropdown menus. Additionally, I suggest that toolbar items should not be highlighted upon clicking, as I only intend to open the dropdown menu.

hamed-musallam commented 7 months ago

On the other hand, I believe the popup animation is slow, can you increase the speed of the animation to have smooth animation?

hamed-musallam commented 7 months ago

it is just a suggestion, can we specify the trigger action as a long click, hover, or click on the dropdown toolbar.

stropitek commented 7 months ago

On the other hand, I believe the popup animation is slow, can you increase the speed of the animation to have smooth animation?

Those are in the CSS of blueprint and so are not editable. But we can use the minimal prop of popover which makes the animation much more subtle. It will also remove the "arrow" that points from the menu to the button.

it is just a suggestion, can we specify the trigger action as a long click, hover, or click on the dropdown toolbar.

I'd be against "long click" since this is difficult to discover and non-standard. What are you using in NMRium? All of those? In the Popover component of blueprint it's possible to use click or hover

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

hamed-musallam commented 7 months ago

I'd be against "long click" since this is difficult to discover and non-standard. What are you using in NMRium? All of those? In the Popover component of blueprint it's possible to use click or hover over

It's just a suggestion, but currently in NMRIUM, we only have a dropdown menu triggered by a click. What I propose is similar to Photoshop's functionality, where tools can be grouped and it is activated by long press

Screenshot 2024-02-06 at 09 56 17

stropitek commented 7 months ago

I don't have a photoshop license but AFAIK it's because clicking selects the default tool in the category, thus the long click to select other related tools. Is that what you'd like for NMRIum.

I'm still skeptical about the long-press. Haven't seen this anywhere in the web I think.

targos commented 7 months ago

The web version of Photoshop doesn't have this behavior. They open the submenus on hover.

stropitek commented 7 months ago

@hamed-musallam actually I was wrong the Popover component has a property transitionDuration. You can set this property on the Toolbar.PopoverItem component.

hamed-musallam commented 7 months ago

thank you @stropitek , i will try it once I update to the latest version of react-science