EZP-29280: As an editor, I want to add anchors to RichText content #831

dew326 · 2019-02-05T08:56:40Z

Question	Answer
Tickets	https://jira.ez.no/browse/EZP-29280
Depends on	ezsystems/ezplatform-richtext#28
Bug fix?	no
New feature?	yes
BC breaks?	no
Tests pass?	yes
Doc needed?	no
License	GPL-2.0

This PR introduces new svg icon link-anchor:

Checklist:

Coding standards ($ composer fix-cs)
Ready for Code Review

sunpietro · 2019-02-12T08:02:07Z

src/bundle/Resources/public/js/alloyeditor/src/buttons/ez-btn-anchor.js

+        const label = Translator.trans(/*@Desc("Anchor")*/ 'anchor_btn.label', {}, 'alloy_editor');
+
+        return (
+            <button


Missing type="button"

Why do we need the type in alloyeditor? None of the buttons from AE has this and I follow their convention.

Unless I'm mistaken the alloy editor app is rendered inside a form. Having type="button" added to a button element secures the app and us from a random form submission attempt.

AE is not rendered in the form. It has own container at the end of the page.

I see. Anyway, it's a good practice to add this attribute, but I'm fine if you want to follow the convention from AE.

sunpietro · 2019-02-12T08:02:45Z

src/bundle/Resources/public/js/alloyeditor/src/buttons/ez-btn-anchor.js

+                aria-pressed={cssClass.indexOf('pressed') !== -1}
+                className={cssClass}
+                onClick={this.props.requestExclusive}
+                tabIndex={this.props.tabIndex}


Do you need to set tabIndex on a button? Can you control the tab switch flow?

I follow the convention from AE

Ok, then it's fine.

sunpietro · 2019-02-12T08:04:21Z

src/bundle/Resources/public/js/alloyeditor/src/buttons/ez-btn-anchoredit.js

+    findIcon() {
+        const block = this.findBlock();
+
+        return [...block.getChildren().$].find((child) => child.classList && child.classList.contains('ez-icon--anchor'));


Can you store it: ez-icon--anchor as a variable?

I would extract arrow function to method to improve readability.

sunpietro · 2019-02-12T09:49:53Z

src/bundle/Resources/public/scss/_alloyeditor-anchor-edit.scss

+    display: flex;
+    padding: calculateRem(8px) 0 calculateRem(8px) calculateRem(8px);
+
+    &__input-wrapper {


What about moving it inside the &__input { selector? In fact, it looks like it should be a separate component, but it's acceptable use case here.

Are you suggesting using this structure:

&__input { /*...*/ &-wrapper {/*...*/} &-label {/*...*/} }

? I have bad experience with splitting selectors in this way. I have once searched for a selector and it took me a while, because element name was split in halve.
For me the most natural and easy to comprehend approach is to split class name in 3: block, element, modifier, like it was done here. 🙂

sunpietro · 2019-02-12T09:51:02Z

src/bundle/Resources/public/scss/_alloyeditor-anchor-edit.scss

+            background-color: $ez-color-positive;
+        }
+
+        &[disabled] {


I'm still thinking about unifying this. Because I found we're adding the similar rule here and there in our codebase. We have to check whether we could unify it and make it more generic.

src/bundle/Resources/public/js/alloyeditor/src/buttons/ez-btn-anchor.js

tischsoic · 2019-02-12T12:22:15Z

src/bundle/Resources/public/js/alloyeditor/src/buttons/ez-btn-anchor.js

+            return <AlloyEditor.EzBtnAnchorEdit {...this.props} />;
+        }
+
+        const cssClass = 'ae-button ez-btn-ae--anchor ez-btn-ae ' + this.getStateClasses();


Nitpick: we usually use `${}` to concatenate strings. Do we have any in-house rule on this? There is eslint rule for this: https://eslint.org/docs/rules/prefer-template.

Yes, most of the button in AE has the convention that I proposed. We should unify this in 3.0.

tischsoic · 2019-02-12T12:29:45Z

src/bundle/Resources/public/js/alloyeditor/src/buttons/ez-btn-anchoredit.js

+    findIcon() {
+        const block = this.findBlock();
+
+        return [...block.getChildren().$].find((child) => child.classList && child.classList.contains('ez-icon--anchor'));


I would extract arrow function to method to improve readability.

tischsoic · 2019-02-12T12:35:34Z

src/bundle/Resources/public/js/alloyeditor/src/buttons/ez-btn-anchoredit.js

+
+    removeAnchor() {
+        const block = this.findBlock();
+        const icon = this.findIcon();


Nitpick: we can pass block to findIcon instead of finding it twice.

But I wanted to have a more atomic method. And if you look atfindBlock() you will find out that the method is calculating things only one time, the second one is returning block immediately.

You're right, I didn't notice this.block = block;. I like your approach! :)

tischsoic · 2019-02-12T12:42:41Z

src/bundle/Resources/public/js/scripts/fieldType/base/base-rich-text.js

@@ -130,6 +130,10 @@
            headers.forEach((header) => header.remove());
        }

+        clearAnchorIcons(icon) {


Nitpick: clearAnchorIcon, without s as we clear only one icon.

katarzynazawada

I have a few comments, tested together with ezsystems/ezplatform-richtext#28

1) Link does not point to anchor when user adds link first and then creates anchor.

Steps:

Add two paragraphs in RTE.
In second paragraph link to anchor, e.g #test.
In first paragraph add anchor (test).
Publish.

Link in second paragraph dose not point to anchor (anchor is not saved)

2) Second unused anchor is not saved.

Steps:

Add anchor to first paragraph in RTE.
Add anchor to second paragraph in RTE.
Publish content.
Edit content

Second anchor is not added.

3) Remove button should be grayed out when editing anchor for the first time or it should remove anchor.

Steps:

Click in RTE
Type some text.
Click on anchor icon.
Type name.
Click on Remove button.

Error in console

4) Missing colon :

5) Half border of Remove and Save buttons (Safari, Firefox, Edge)

6) One extra line when adding anchor in Firefox

Steps:

Type some text in RTE.
Click on anchor icon, fill name and save.
Click in RTE area.

7) No anchor icon when formatted style is selected

Steps:

Click in RTE
Change style from Paragraph to Formatted.
Type some text.
Click anchor icon
Type name and save

Anchor icon is not displayed. Moreover, one additional line is added.

8) When pressing Enter button in paragraph with anchor, icon skips to next line

Steps:

Type some text in RTE.
Click on anchor icon, fill name and save.
Press Enter button.

In console:

9) Anchor to image does not work.

Steps:

Add image in RTE.
Click on anchor icon, fill name and save.
In other paragraph link to anchor.
Publish.

When clicking on created link user is not pointed to anchor.

10) Error when editing anchor in image.

Steps:

Add image to RTE.
Click on anchor icon, fill name and save.
Click on anchor icon again.

Error occurs:

11) Anchor icon changes position in table.

Steps:

Add table in RTE.
Click on anchor icon, fill name and save. (A)
In other paragraph link to anchor.
Publish.
Edit. (B)

A

B

12) Embed inline in anchor (Chrome and Safari)

Steps:

Type some text in RTE.
Click on anchor icon, fill name and save.
Add embed inline.

Cursor is moved to the beginning of the paragraph, it is not possible to write text after the embed block

13) Validation of XML content failed when adding two anchors with the same name

Steps:

Add anchor to first paragraph.
Add anchor to second paragraph with the same name.
In third paragraph add link to anchor.
Publish.

Error Validation of XML content failed: Error in 40:0: Element section has extra content: para. occurs.
Mabye error message should be more user-friendly ? Or it could be validated earlier – when adding name.

micszo

Issues 1-9 and 12 do not occur anymore.
It was agreed that 10, 11 and 13 will be handled separately (~~will add tickets~~ linked to story).
Also 7 and 9 require little follow-ups.

…zsystems#831) * EZP-29280: As an editor, I want to add anchors to RichText content * compiled AE

#831) [Behat] Added small wait before interacting with Single relation button (#830)

dew326 self-assigned this Feb 5, 2019

dew326 added the Feature label Feb 5, 2019

dew326 changed the title ~~[WIP] EZP-29280: As an editor, I want to add anchors to RichText content~~ EZP-29280: As an editor, I want to add anchors to RichText content Feb 11, 2019

dew326 requested review from sunpietro and tischsoic February 11, 2019 12:48

sunpietro suggested changes Feb 12, 2019

View reviewed changes

tischsoic requested changes Feb 12, 2019

View reviewed changes

adamwojs mentioned this pull request Feb 13, 2019

EZP-29280: As an editor, I want to add anchors to RichText content ezsystems/ezplatform-richtext#28

Merged

4 tasks

tischsoic approved these changes Feb 15, 2019

View reviewed changes

katarzynazawada self-assigned this Feb 18, 2019

katarzynazawada requested changes Feb 19, 2019

View reviewed changes

sunpietro approved these changes Feb 19, 2019

View reviewed changes

dew326 force-pushed the anchor-in-richtext branch 2 times, most recently from 06ddefc to d5f4776 Compare February 25, 2019 14:07

dew326 added 2 commits February 25, 2019 15:09

EZP-29280: As an editor, I want to add anchors to RichText content

acd8096

compiled AE

3a3c395

dew326 force-pushed the anchor-in-richtext branch from d5f4776 to 3a3c395 Compare February 25, 2019 14:10

micszo self-assigned this Feb 26, 2019

micszo approved these changes Feb 26, 2019

View reviewed changes

lserwatka merged commit bf0bfcd into ezsystems:master Feb 26, 2019

micszo removed their assignment Feb 26, 2019

konradoboza pushed a commit to konradoboza/ezplatform-admin-ui that referenced this pull request May 29, 2019

EZP-29280: As an editor, I want to add anchors to RichText content (e…

2f6dbeb

…zsystems#831) * EZP-29280: As an editor, I want to add anchors to RichText content * compiled AE

mateuszdebinski pushed a commit that referenced this pull request Feb 13, 2024

[Behat] Added small wait before interacting with Single relation butt… (

22bd8ac

#831) [Behat] Added small wait before interacting with Single relation button (#830)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EZP-29280: As an editor, I want to add anchors to RichText content #831

EZP-29280: As an editor, I want to add anchors to RichText content #831

dew326 commented Feb 5, 2019 •

edited by tischsoic

Loading

sunpietro Feb 12, 2019

dew326 Feb 15, 2019

sunpietro Feb 15, 2019

dew326 Feb 15, 2019

sunpietro Feb 15, 2019

sunpietro Feb 12, 2019

dew326 Feb 15, 2019

sunpietro Feb 15, 2019

sunpietro Feb 12, 2019

tischsoic Feb 12, 2019

sunpietro Feb 12, 2019

tischsoic Feb 12, 2019

sunpietro Feb 12, 2019

tischsoic Feb 12, 2019

dew326 Feb 15, 2019

tischsoic Feb 12, 2019

tischsoic Feb 12, 2019

dew326 Feb 15, 2019

tischsoic Feb 15, 2019

tischsoic Feb 12, 2019

katarzynazawada left a comment

micszo left a comment •

edited

Loading

EZP-29280: As an editor, I want to add anchors to RichText content #831

EZP-29280: As an editor, I want to add anchors to RichText content #831

Conversation

dew326 commented Feb 5, 2019 • edited by tischsoic Loading

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

katarzynazawada left a comment

Choose a reason for hiding this comment

micszo left a comment • edited Loading

Choose a reason for hiding this comment

dew326 commented Feb 5, 2019 •

edited by tischsoic

Loading

micszo left a comment •

edited

Loading