feat(UI): Improve split button styling #10027

maks-w · 2024-09-10T15:12:42Z

Change-Id: I4f0474e02411ada5547c8c2f23dda22cb4433557

Resolves: Improve split button styling #8566
Target version: master

Summary

This is my proposal for UI splitting the buttons in 2 parts.
I hope I didn't take too much liberty with the design ^^

Checklist

I have run make prettier-write and formatted the code.
All commits have Change-Id
I have run tests with make check
I have issued make run and manually verified that everything looks okay
Documentation (manuals or wiki) has been updated or is not required

browser/src/control/Control.JSDialogBuilder.js

maks-w · 2024-09-12T12:31:17Z

browser/css/btns.css

 .jsdialog.unotoolbutton.selected:hover {
-	background-color: var(--color-background-darker) !important;


We move the responsability of the background-color to the children since a button could have 2 parts

maks-w · 2024-09-12T12:32:42Z

browser/css/jsdialogs.css

+}
+
+.unoarrow::after,
+.jsdialog .ui-content.unobutton::after {


I extended a little bit the click zone so that the border of the button could be clickable

maks-w · 2024-09-12T12:35:46Z

cypress_test/integration_tests/desktop/impress/delete_objects_spec.js

@@ -37,6 +37,7 @@ describe(['tagdesktop', 'tagnextcloud', 'tagproxy'], 'Delete Objects', function(
 	});

 	it('Delete Chart' , function() {
+		cy.cGet('#toolbar-up > .ui-scroll-right').click();
 		cy.cGet('#toolbar-up > .ui-scroll-right').click();


The menu is a bit wider now, so we need to click twice for the icon to appear.

eszkadev · 2024-09-17T08:43:07Z

I see no problem with the JS code thanks! @pedropintosilva will you find a minute to review the new look of split button?

pedropintosilva

thanks @maks-w ! It looks much better, well done 👏

Before we can merge this it would be good to understand why we are forgoing the --btn-size CSS var. That was a nice way to not only make sure all icon buttons share the same size but also to allow adjusting them in one single place. With the current changes we are never sure about the resulting size.

Also the pseudo elements in other buttons are being affected (not only arrows). Example the save icon in the tabbed bar (when it receives the .savemodified class. Meaning when the user types and there are changes ready to be saved)

eszkadev · 2024-10-10T15:37:09Z

browser/src/control/Control.JSDialogBuilder.js

@@ -2760,8 +2760,8 @@ L.Control.JSDialogBuilder = L.Control.extend({

 			if (typeof menubutton === 'object') {
 				L.DomUtil.addClass(menubutton.container, data.class ? data.class + ' has-colorpicker': 'has-colorpicker');
-
-				var valueNode = L.DomUtil.create('div', 'selected-color', menubutton.container);
+


can we remove this white space?

maks-w · 2024-10-14T08:42:49Z

thanks @maks-w ! It looks much better, well done 👏

Before we can merge this it would be good to understand why we are forgoing the --btn-size CSS var. That was a nice way to not only make sure all icon buttons share the same size but also to allow adjusting them in one single place. With the current changes we are never sure about the resulting size.

I pushed a new commit to restore this behavior. Some changes however, because of the padding, the button and the image are not the same size now, so there is a button size and an image size variable (the image size is computed, so there is still only 1 variable to change to change the size of the button).

I took the liberty to reduce a little bit the size of the buttons, who seem a little too big (but perhaps I took it a step too far). Tell me what you think.

Also the pseudo elements in other buttons are being affected (not only arrows). Example the save icon in the tabbed bar (when it receives the .savemodified class. Meaning when the user types and there are changes ready to be saved)

For the pseudo element, it seems to me that it is a good practice to extend a little bit the click zone of buttons in general; not just for the arrow button.
Even today we have the rollover actif before we can click on it, which can cause missclicks.
But maybe I didn't understand correctly the issue?

I just noticed that some icons are blurry with the added padding :(
Let me look into it and check why the cypress tests are not green anymore.

eszkadev · 2024-10-14T11:17:47Z

please rebase, I think tests are failing due to old base

it was broken after PR: #10027 I'm afraid I was not as clear as I could have been, sorry #10027 (review) What I mean is that it's very important to: * When fixing/improving the style of a given widget to avoid pushing additional changes that have a high risk of changing many things and bring regressions * I welcome those changes but in an isolated PR which can better tested and worked on :) * Changing sizes of buttons or let them be dynamically set according to the DOM is a bit hard and prone to problems Nevertheless this is an attempt to fix those while retain the nice work from Maks. Co-authored-by: Szymon Kłos <[email protected]> Signed-off-by: Pedro Pinto Silva <[email protected]> Change-Id: I2d6aa5113405aa32e06d45cfbfd1a623647f3121

it was broken after PR: CollaboraOnline/online#10027 I'm afraid I was not as clear as I could have been, sorry CollaboraOnline/online#10027 (review) What I mean is that it's very important to: * When fixing/improving the style of a given widget to avoid pushing additional changes that have a high risk of changing many things and bring regressions * I welcome those changes but in an isolated PR which can better tested and worked on :) * Changing sizes of buttons or let them be dynamically set according to the DOM is a bit hard and prone to problems Nevertheless this is an attempt to fix those while retain the nice work from Maks. Co-authored-by: Szymon Kłos <[email protected]> Signed-off-by: Pedro Pinto Silva <[email protected]> Change-Id: I2d6aa5113405aa32e06d45cfbfd1a623647f3121

Issue that came up with its follow ups db10c45 #10027 Signed-off-by: Pedro Pinto Silva <[email protected]> Change-Id: I6c9401e9f3a37fd604a2598388be64f026efce21

Due to db10c45 #10027 we now need to make sure the img element is completely out of the way via padding otherwise the background-image is not visible Signed-off-by: Pedro Pinto Silva <[email protected]> Change-Id: Idf909b4cffbedf71b75797a6611fb8067f2a314a

it was broken after PR: #10027 I'm afraid I was not as clear as I could have been, sorry #10027 (review) What I mean is that it's very important to: * When fixing/improving the style of a given widget to avoid pushing additional changes that have a high risk of changing many things and bring regressions * I welcome those changes but in an isolated PR which can better tested and worked on :) * Changing sizes of buttons or let them be dynamically set according to the DOM is a bit hard and prone to problems Nevertheless this is an attempt to fix those while retain the nice work from Maks. Co-authored-by: Szymon Kłos <[email protected]> Signed-off-by: Pedro Pinto Silva <[email protected]> Change-Id: I2d6aa5113405aa32e06d45cfbfd1a623647f3121

Issue that came up with its follow ups db10c45 #10027 Signed-off-by: Pedro Pinto Silva <[email protected]> Change-Id: I6c9401e9f3a37fd604a2598388be64f026efce21

Due to db10c45 #10027 we now need to make sure the img element is completely out of the way via padding otherwise the background-image is not visible Signed-off-by: Pedro Pinto Silva <[email protected]> Change-Id: Idf909b4cffbedf71b75797a6611fb8067f2a314a

it was broken after PR: #10027 I'm afraid I was not as clear as I could have been, sorry #10027 (review) What I mean is that it's very important to: * When fixing/improving the style of a given widget to avoid pushing additional changes that have a high risk of changing many things and bring regressions * I welcome those changes but in an isolated PR which can better tested and worked on :) * Changing sizes of buttons or let them be dynamically set according to the DOM is a bit hard and prone to problems Nevertheless this is an attempt to fix those while retain the nice work from Maks. Co-authored-by: Szymon Kłos <[email protected]> Signed-off-by: Pedro Pinto Silva <[email protected]> Change-Id: I2d6aa5113405aa32e06d45cfbfd1a623647f3121 Signed-off-by: Darshan-upadhyay1110 <[email protected]>

Issue that came up with its follow ups db10c45 #10027 Signed-off-by: Pedro Pinto Silva <[email protected]> Change-Id: I6c9401e9f3a37fd604a2598388be64f026efce21 Signed-off-by: Darshan-upadhyay1110 <[email protected]>

Due to db10c45 #10027 we now need to make sure the img element is completely out of the way via padding otherwise the background-image is not visible Signed-off-by: Pedro Pinto Silva <[email protected]> Change-Id: Idf909b4cffbedf71b75797a6611fb8067f2a314a Signed-off-by: Darshan-upadhyay1110 <[email protected]>

Issue that came up with its follow ups db10c453e95f732d85c489fe8da8708cec0ad09c CollaboraOnline/online#10027 Signed-off-by: Pedro Pinto Silva <[email protected]> Change-Id: I6c9401e9f3a37fd604a2598388be64f026efce21

Due to db10c453e95f732d85c489fe8da8708cec0ad09c CollaboraOnline/online#10027 we now need to make sure the img element is completely out of the way via padding otherwise the background-image is not visible Signed-off-by: Pedro Pinto Silva <[email protected]> Change-Id: Idf909b4cffbedf71b75797a6611fb8067f2a314a