Buttons separators in RWD mobile should not be full height #8232

pkwasnik · 2020-10-09T06:58:35Z

Suggested merge commit message (convention)

Fix (link): Implementing not full height separators. Closes #7704.

Additional information

For example – encountered issues, assumptions you had to make, other affected tickets, etc.

jodator

I'm not sure about the approach, so I would double-check this with @oleq .

Apart from that 👍 for picking up tests in the right way for the added element.

jodator · 2020-10-09T07:19:32Z

packages/ckeditor5-link/src/ui/linkformview.js

@@ -24,6 +24,7 @@ import KeystrokeHandler from '@ckeditor/ckeditor5-utils/src/keystrokehandler';
 import checkIcon from '@ckeditor/ckeditor5-core/theme/icons/check.svg';
 import cancelIcon from '@ckeditor/ckeditor5-core/theme/icons/cancel.svg';
 import '../../theme/linkform.css';
+import ToolbarSeparatorView from '@ckeditor/ckeditor5-ui/src/toolbar/toolbarseparatorview';


This import should go alongside other code imports, preferably glued together with similar imports (ie after LabeledFieldVIew import).

We like tidy imports :D

right, corrected

jodator · 2020-10-09T07:21:53Z

packages/ckeditor5-link/src/ui/linkformview.js

@@ -85,6 +86,8 @@ export default class LinkFormView extends View {
 		 */
 		this.cancelButtonView = this._createButton( t( 'Cancel' ), cancelIcon, 'ck-button-cancel', 'cancel' );

+		this.separatorView = new ToolbarSeparatorView();


I think that this is not a good way to go. The ToolbarSeparatorView is used (AFAIR for now) only inside the toolbar.

Maybe here we could just adjust buttons' CSS to achieve the required UI.

// cc @oleq

jodator · 2020-10-09T07:27:26Z

packages/ckeditor5-link/src/ui/linkformview.js

@@ -331,6 +334,7 @@ export default class LinkFormView extends View {
 		}

 		children.add( this.saveButtonView );
+		children.add( this.separatorView );


I've fortgot to check that also the main view for the links needs update:

ckeditor5/packages/ckeditor5-link/src/ui/linkactionsview.js

Lines 127 to 131 in 45f40e5

children: [

this.previewButtonView,

this.editButtonView,

this.unlinkButtonView

]

Sorry for that - I've figured out this after looking how this behaves. So we heve here:

LinkActionsView

LinkFormView

jodator · 2020-10-09T07:30:54Z

packages/ckeditor5-link/theme/linkform.css

+			margin-top: calc(var(--ck-spacing-large) + var(--ck-spacing-extra-tiny));
+			margin-bottom: var(--ck-spacing-small);


Something looks odd when I've tested it locally. See the screenshot for LinkFormView.

Oh, I've found that it looks OK if no manual decorators are defined:

oleq

Aside from the fact that the solution breaks in certain situations

I think this direction is a dead-end. Some more thoughts:

The action buttons are not a toolbar so using ToolbarSeparatorView in this context is a no-go.
We cannot convert these buttons into a toolbar because... they are not a toolbar. Toolbars carry specific semantics and this is not the case.
I'd see what we can do using ::before or ::after pseudo-selectors for the buttons (pure CSS approach). For instance .ck-button::after:

border-right: 1px solid var(--ck-color-base-border);
content: "";
width: 0;
position: absolute;
right: -1px;
top: var(--ck-spacing-small);
bottom: var(--ck-spacing-small);
z-index: 1;

results in something like this (ignore the broken toggle switches, it's just a PoC and I didn't narrow the selector)

This reverts commit 856ea72.

This reverts commit 64fa3cc.

This reverts commit 5d22fa5.

jodator

The mobile views are now OK 👍

However, the separator is now lost in manual decorators samples

PR	master

@oleq - Previously we had this style copied 3 times (I see 3 deletions in this PR). For a single line style, it doesn't look like a big overhead. Adding more properties means that we have to copy them in 3 places. Do we have any strategy or pattern for duplicated definitions?

oleq · 2020-10-14T09:16:22Z

Adding more properties means that we have to copy them in 3 places. Do we have any strategy or pattern for duplicated definitions?

Yeah. Normally we create a class and then it's shared across the use-cases. The story here is tricky, though because there's also the text alternative form that needs to be addressed and that means the styles must be shared across packages and cannot live in ckeditor5-link or ckeditor5-image.

Some cake recipe (not sure it tastes good, though):

Let's have .ck-responsive-form together with .ck-text-alternative-form, .ck-link-form, and .ck-link-actions (in TextAlternativeFormView, LinkFormView, LinkActionsView),
Let's have ckeditor5-ui/theme/components/responsive-form/responsiveform.css (for wireframe part),
Let's have ckeditor5-theme-lark/theme/ckeditor5-ui/components/responsive-form/responsiveform.css (for theme part). ATM both wireframe and theme CSS are in the same file. I really need to address Document and explain CSS organization in the project #7702 to make it clear, sorry for that 😞
Let's make the &::after { ... } solution work as .ck-responsive-form some-sane-selector-that-reaches-the-correct-button::after { ... } both in responsive mode and when .ck-link-form_layout-vertical is used. This is tricky and needs to be thought through.
- It could be we need something like this:

/* Either of responsiveform.css */

/* This one can be used here https://github.com/ckeditor/ckeditor5/blob/0706215c592edcf1b701b4af63298f4b77f8dd01/packages/ckeditor5-link/src/ui/linkformview.js#L139 */
/* It simply enforces the mode when the class is set. */
.ck-vertical-form some-sane-selector-that-reaches-the-correct-button::after {
	/* Some CSS. */
}

/* This one is smart and works based on media queries. */
@mixin ck-media-phone {
	.ck-responsive-form some-sane-selector-that-reaches-the-correct-button::after {
		/* Same CSS as above. */
	}
}

It all should work as an equivalent to code decorator pattern except that it's just CSS. There will be no JS (e.g. ResponsiveFormView) for the component, just CSS injected (first imported) via .ck-responsive-form class.
We should make all the RWD/vertical styles common for these 3 forms in responsiveform.css, for instance, https://github.com/ckeditor/ckeditor5/blob/0706215c592edcf1b701b4af63298f4b77f8dd01/packages/ckeditor5-theme-lark/theme/ckeditor5-link/linkform.css#L29-L66.
- I struggle to picture this all in my head without creating an actual code, though. But I think you got the idea.

jodator · 2020-10-14T10:23:31Z

I really need to address #7702 to make it clear, sorry for that

It would be lovely if you could manage to find time for that 😍 having those written down would also mean fewer pings from others 😉

jodator

LGTM 👍 Only one request to @pkwasnik about documenting the button.ck-button usage and we can merge it.

jodator · 2020-10-19T13:20:35Z

packages/ckeditor5-theme-lark/theme/ckeditor5-ui/components/responsive-form/responsiveform.css

+			}
+		}
+
+		& button.ck-button {


@pkwasnik Could you add a comment on why there's button.ck-button is needed. It wasn't obvious to me why this is needed.

jodator

LGTM 👍

Buttons separators in RWD mobile should not be full height

5d22fa5

pkwasnik requested a review from jodator October 9, 2020 06:58

Added comment

64fa3cc

jodator self-assigned this Oct 9, 2020

jodator suggested changes Oct 9, 2020

View reviewed changes

Refactoring

856ea72

oleq self-requested a review October 12, 2020 07:55

Merge branch 'master' into i/7704

7bc7070

oleq requested changes Oct 12, 2020

View reviewed changes

pkwasnik added 4 commits October 12, 2020 15:17

Revert "Refactoring"

7c73016

This reverts commit 856ea72.

Revert "Added comment"

a2001d5

This reverts commit 64fa3cc.

Revert "Buttons separators in RWD mobile should not be full height"

0c25b60

This reverts commit 5d22fa5.

Change link tooltip button separator design

11f14a3

jodator suggested changes Oct 13, 2020

View reviewed changes

pkwasnik added 2 commits October 13, 2020 09:35

Change link tooltip button separator design for manual decorators

96011f9

Merge branch 'master' into i/7704

808066b

pkwasnik added 4 commits October 18, 2020 21:32

Changed button separator length for text alternative form

f728cb0

Moving new separator styles to 'ckeditor5-ui'

9e5a7a7

Moving new separator styles to 'ckeditor5-ui'

9e0065e

Added copyright

ec432ca

jodator approved these changes Oct 19, 2020

View reviewed changes

pkwasnik added 2 commits October 19, 2020 22:15

Added tests

af1aab9

Added comment

172a544

jodator approved these changes Oct 20, 2020

View reviewed changes

jodator merged commit 6aecaf8 into master Oct 20, 2020

jodator deleted the i/7704 branch October 20, 2020 09:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Buttons separators in RWD mobile should not be full height #8232

Buttons separators in RWD mobile should not be full height #8232

pkwasnik commented Oct 9, 2020

jodator left a comment

jodator Oct 9, 2020

pkwasnik Oct 9, 2020

jodator Oct 9, 2020

jodator Oct 9, 2020

jodator Oct 9, 2020

jodator Oct 9, 2020

oleq left a comment

jodator left a comment

oleq commented Oct 14, 2020

jodator commented Oct 14, 2020

jodator left a comment

jodator Oct 19, 2020

jodator left a comment

	children: [
	this.previewButtonView,
	this.editButtonView,
	this.unlinkButtonView
	]

		margin-top: calc(var(--ck-spacing-large) + var(--ck-spacing-extra-tiny));
		margin-bottom: var(--ck-spacing-small);

Buttons separators in RWD mobile should not be full height #8232

Buttons separators in RWD mobile should not be full height #8232

Conversation

pkwasnik commented Oct 9, 2020

Suggested merge commit message (convention)

Additional information

jodator left a comment

Choose a reason for hiding this comment

jodator Oct 9, 2020

Choose a reason for hiding this comment

pkwasnik Oct 9, 2020

Choose a reason for hiding this comment

jodator Oct 9, 2020

Choose a reason for hiding this comment

jodator Oct 9, 2020

Choose a reason for hiding this comment

jodator Oct 9, 2020

Choose a reason for hiding this comment

jodator Oct 9, 2020

Choose a reason for hiding this comment

oleq left a comment

Choose a reason for hiding this comment

jodator left a comment

Choose a reason for hiding this comment

oleq commented Oct 14, 2020

jodator commented Oct 14, 2020

jodator left a comment

Choose a reason for hiding this comment

jodator Oct 19, 2020

Choose a reason for hiding this comment

jodator left a comment

Choose a reason for hiding this comment