SAP / fundamental-ngx

Fundamental Library for Angular is SAP Design System Angular component library
https://sap.github.io/fundamental-ngx
Apache License 2.0
257 stars 125 forks source link

feat: introduce toolbar component #2657

Closed rengare closed 3 years ago

rengare commented 3 years ago

Please provide a link to the associated issue.

Closes #2451

Please provide a brief summary of this pull request.

The PR brings fiori3 toolbar component to ngx

Please check whether the PR fulfills the following requirements

Documentation checklist:

netlify[bot] commented 3 years ago

Deploy preview for fundamental-ngx ready!

Built with commit 41badf734618abbd0cb563b79d253f5bb1984d8d

https://deploy-preview-2657--fundamental-ngx.netlify.app

mikerodonnell89 commented 3 years ago

There is a difference between "info" state and "active" state in styles and they seem to be merged into the same thing here

mikerodonnell89 commented 3 years ago

I think we need directives to apply the classes fd-toolbar__overflow__label and fd-toolbar__overflow__form-label to stuff that goes in the overflow menu, see styles example

mikerodonnell89 commented 3 years ago

Probably should add some margin to the overflow button to prevent this from happening. The "___ more" tokenizer might address this problem somehow, i can't remember

Screen Shot 2020-06-12 at 3 39 20 PM

rengare commented 3 years ago

Probably should add some margin to the overflow button to prevent this from happening. The "___ more" tokenizer might address this problem somehow, i can't remember

Screen Shot 2020-06-12 at 3 39 20 PM

I added extra space here

rengare commented 3 years ago

Transparent toolbars need the 'fd-toolbar--clear' class to remove the bottom border, and it should be optional for auto toolbars

Screen Shot 2020-06-12 at 3 29 38 PM

I added property for that

mikerodonnell89 commented 3 years ago

Still missing the distinction between "info" and "active" state

mikerodonnell89 commented 3 years ago

Resizing overflow menu should be able to move separators into the menu as well, like on styles:

Screen Shot 2020-06-15 at 10 13 57 AM

vs. here: Screen Shot 2020-06-15 at 10 20 29 AM

stefanoScalzo commented 3 years ago

Need to fix the separator with the overflow. I shrank the page and it is not there

rengare commented 3 years ago

Still missing the distinction between "info" and "active" state

fixed

rengare commented 3 years ago

Need to fix the separator with the overflow. I shrank the page and it is not there

fixed

rengare commented 3 years ago

Resizing overflow menu should be able to move separators into the menu as well, like on styles:

Screen Shot 2020-06-15 at 10 13 57 AM

vs. here: Screen Shot 2020-06-15 at 10 20 29 AM

fixed

mikerodonnell89 commented 3 years ago

This may be up for debate, in which case, I am happy moving forward with this PR and addressing my comments in another issue/PR. But I wanted to point this out.

There is some unpredictable behavior with how items are removed from the toolbar and placed into the overflow container, and vice versa. Copy this html to the overflow example

<fd-toolbar [shouldOverflow]="true">
    <button fd-toolbar-item fd-button [compact]="true">Button 1</button>
    <label fd-toolbar-label fd-toolbar-item>text 1</label>
    <fd-toolbar-separator fd-toolbar-item></fd-toolbar-separator>
    <button fd-toolbar-item fd-button [compact]="true">Button 2</button>
    <label fd-toolbar-label fd-toolbar-item>text 2</label>
    <fd-toolbar-separator fd-toolbar-item></fd-toolbar-separator>
    <button fd-toolbar-item fd-button [compact]="true">Button 3</button>
    <label fd-toolbar-label fd-toolbar-item>text 3</label>
    <button fd-toolbar-item fd-button [compact]="true">Button 4</button>
    <label fd-toolbar-label fd-toolbar-item>text 4</label>
    <fd-toolbar-spacer></fd-toolbar-spacer>
    <button fd-toolbar-item fd-button [compact]="true">Button 5</button>
    <label fd-toolbar-label fd-toolbar-item>text 5</label>
    <fd-toolbar-spacer></fd-toolbar-spacer>
    <fd-toolbar-separator fd-toolbar-item></fd-toolbar-separator>
    <button fd-toolbar-item fd-button [compact]="true">Button 6</button>
    <label fd-toolbar-label fd-toolbar-item>text 6</label>
    <fd-toolbar-separator fd-toolbar-item></fd-toolbar-separator>
    <button fd-toolbar-item fd-button [compact]="true">Button 7</button>
    <label fd-toolbar-label fd-toolbar-item>text 7</label>
    <button fd-toolbar-item fd-button [compact]="true">Button 8</button>
    <label fd-toolbar-label fd-toolbar-item>text 8</label>
    <fd-toolbar-spacer></fd-toolbar-spacer>
    <button fd-toolbar-item fd-button [compact]="true">Button 9</button>
    <label fd-toolbar-label fd-toolbar-item>text 9</label>
    <fd-toolbar-spacer></fd-toolbar-spacer>
    <fd-toolbar-separator fd-toolbar-item></fd-toolbar-separator>
    <button fd-toolbar-item fd-button [compact]="true">Button 10</button>
    <label fd-toolbar-label fd-toolbar-item>text 10</label>
    <fd-toolbar-separator fd-toolbar-item></fd-toolbar-separator>
    <button fd-toolbar-item fd-button [compact]="true">Button 11</button>
    <label fd-toolbar-label fd-toolbar-item>text 11</label>
    <button fd-toolbar-item fd-button [compact]="true">Button 12</button>
    <label fd-toolbar-label fd-toolbar-item>text 12</label>
    <fd-toolbar-spacer></fd-toolbar-spacer>
</fd-toolbar>

As you can see it goes button 1, text 1, button 2, text 2, etc.

In this gif I start resizing the window and you can see text 6 is hidden, then when button 6 gets hidden, text 6 is visible back in the toolbar. I think items should be added to the overflow list sequentially, and what is happening here is that when button 6 is collapsed, this has freed up enough space for text 6 to come back, so it does. I think in this case text 6 should remain in the overflow menu. Otherwise, the order of items in the overflow menu does not correspond to how the application developer arranged them in the toolbar.

toolbarresize

Another example of this logic resulting in some weird behavior is here, where you will see multiple spacers next to each other in the toolbar, even though that's not how they're arranged in the HTML.

Screen Shot 2020-06-18 at 4 08 57 PM

mikerodonnell89 commented 3 years ago

This may be up for debate, in which case, I am happy moving forward with this PR and addressing my comments in another issue/PR. But I wanted to point this out.

There is some unpredictable behavior with how items are removed from the toolbar and placed into the overflow container, and vice versa. Copy this html to the overflow example

<fd-toolbar [shouldOverflow]="true">
    <button fd-toolbar-item fd-button [compact]="true">Button 1</button>
    <label fd-toolbar-label fd-toolbar-item>text 1</label>
    <fd-toolbar-separator fd-toolbar-item></fd-toolbar-separator>
    <button fd-toolbar-item fd-button [compact]="true">Button 2</button>
    <label fd-toolbar-label fd-toolbar-item>text 2</label>
    <fd-toolbar-separator fd-toolbar-item></fd-toolbar-separator>
    <button fd-toolbar-item fd-button [compact]="true">Button 3</button>
    <label fd-toolbar-label fd-toolbar-item>text 3</label>
    <button fd-toolbar-item fd-button [compact]="true">Button 4</button>
    <label fd-toolbar-label fd-toolbar-item>text 4</label>
    <fd-toolbar-spacer></fd-toolbar-spacer>
    <button fd-toolbar-item fd-button [compact]="true">Button 5</button>
    <label fd-toolbar-label fd-toolbar-item>text 5</label>
    <fd-toolbar-spacer></fd-toolbar-spacer>
    <fd-toolbar-separator fd-toolbar-item></fd-toolbar-separator>
    <button fd-toolbar-item fd-button [compact]="true">Button 6</button>
    <label fd-toolbar-label fd-toolbar-item>text 6</label>
    <fd-toolbar-separator fd-toolbar-item></fd-toolbar-separator>
    <button fd-toolbar-item fd-button [compact]="true">Button 7</button>
    <label fd-toolbar-label fd-toolbar-item>text 7</label>
    <button fd-toolbar-item fd-button [compact]="true">Button 8</button>
    <label fd-toolbar-label fd-toolbar-item>text 8</label>
    <fd-toolbar-spacer></fd-toolbar-spacer>
    <button fd-toolbar-item fd-button [compact]="true">Button 9</button>
    <label fd-toolbar-label fd-toolbar-item>text 9</label>
    <fd-toolbar-spacer></fd-toolbar-spacer>
    <fd-toolbar-separator fd-toolbar-item></fd-toolbar-separator>
    <button fd-toolbar-item fd-button [compact]="true">Button 10</button>
    <label fd-toolbar-label fd-toolbar-item>text 10</label>
    <fd-toolbar-separator fd-toolbar-item></fd-toolbar-separator>
    <button fd-toolbar-item fd-button [compact]="true">Button 11</button>
    <label fd-toolbar-label fd-toolbar-item>text 11</label>
    <button fd-toolbar-item fd-button [compact]="true">Button 12</button>
    <label fd-toolbar-label fd-toolbar-item>text 12</label>
    <fd-toolbar-spacer></fd-toolbar-spacer>
</fd-toolbar>

As you can see it goes button 1, text 1, button 2, text 2, etc.

In this gif I start resizing the window and you can see text 6 is hidden, then when button 6 gets hidden, text 6 is visible back in the toolbar. I think items should be added to the overflow list sequentially, and what is happening here is that when button 6 is collapsed, this has freed up enough space for text 6 to come back, so it does. I think in this case text 6 should remain in the overflow menu. Otherwise, the order of items in the overflow menu does not correspond to how the application developer arranged them in the toolbar.

toolbarresize

Another example of this logic resulting in some weird behavior is here, where you will see multiple spacers next to each other in the toolbar, even though that's not how they're arranged in the HTML.

Screen Shot 2020-06-18 at 4 08 57 PM

mikerodonnell89 commented 3 years ago

Also that gif demonstrates an issue where the more button is removed from view as the resize is happening

droshev commented 3 years ago

Also that gif demonstrates an issue where the more button is removed from view as the resize is happening

I would say let's try to fix that issue in this PR

rengare commented 3 years ago

Also that gif demonstrates an issue where the more button is removed from view as the resize is happening

This problem shows because of spacer. In this case spacer takes remaining space which means when button is removed from content and moved to overflow, spacer takes button's space. To be honest I don't know how to fix it

mikerodonnell89 commented 3 years ago

I vote for merging this so we can use it in table and opening separate issues for the resizing strangeness. Overflow behavior is not required for most of table's requirements re: toolbar

stefanoScalzo commented 3 years ago

Screen Shot 2020-06-23 at 9 55 49 AM There should be spacing here

stefanoScalzo commented 3 years ago

Add knobs for clear border, active and the text of the buttons in storybook

mikerodonnell89 commented 3 years ago

@rengare if it is OK with you, I'd like to pick up this branch where you have currently left off, I am working on a solution to the flickering on resize

rengare commented 3 years ago

@rengare if it is OK with you, I'd like to pick up this branch where you have currently left off, I am working on a solution to the flickering on resize

Yes, you can work on that, hoverer I added fade in/out animation when visibility is changed. Also, I added debounce for the resizing event. The biggest problem is that I have to move items from overflow to content because in overflow items might have different width (and still there is a spacer with unknown width).