Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

EZP-31231: Fixed storing align and anchor attributes for custom styles #166

Merged
merged 9 commits into from
Oct 7, 2020

Conversation

barw4
Copy link
Member

@barw4 barw4 commented Oct 1, 2020

Question Answer
JIRA issue EZP-31231
Bug yes
New feature no
Target version 1.1
BC breaks no
Tests pass yes
Doc needed no

Align and anchor attributes were not storing and displaying properly when working with custom style, also additional option to render id attribute has been added in form of:

<h2 id="{{ id }}" style="text-align:{{ align }}; color:red">
    {{ content }}
</h2>

where the code above is some custom style.

QA

// maintainer update:

  • Custom Tags and Styles share markup and codebase, so regressions should be checked for both of them.

TODO:

  • Fix a bug.
  • Implement tests.
  • Fix new code according to Coding Standards ($ composer fix-cs).
  • Ask for Code Review.

@barw4 barw4 added the Bug Something isn't working label Oct 1, 2020
@barw4 barw4 self-assigned this Oct 1, 2020
Copy link
Member

@konradoboza konradoboza left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

Co-authored-by: Konrad Oboza <34310128+konradoboza@users.noreply.github.com>
Copy link
Member

@alongosz alongosz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure why you needed to change edit mode output, we only needed to expose that for view output custom template.
The change to edit output is a BC break.

@barw4 barw4 requested review from dew326 and alongosz October 2, 2020 11:03
Copy link
Member

@alongosz alongosz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To sum up #166 (comment):
Custom Tag alignment with the current changes stopped working.
Custom Style (eztemplate of "style" type) can have text-align instead of data-ezalign.
Custom Tag still needs to have data-ezalign.

@barw4
Copy link
Member Author

barw4 commented Oct 5, 2020

@alongosz resolved in 9e86ab3

@barw4 barw4 requested a review from alongosz October 5, 2020 15:33
@barw4 barw4 requested a review from alongosz October 6, 2020 13:18
@katarzynazawada
Copy link

@barw4, storing align and anchor works fine but user cannot link to saved anchor
Steps:

  1. Create or edit a content with a rich text field.
  2. In the rich text field, insert a new custom style element.
  3. Add anchor on the custom style element.
  4. In another paragraph add link to saved anchor.
  5. Publish content.

Result: Clicking on the link does not lead to the place of adding the anchor.

I am not sure, is it in scope of this PR and customer request or it should be reported separately?

@barw4
Copy link
Member Author

barw4 commented Oct 7, 2020

@katarzynazawada

In order for anchor to work properly one must include it in the custom style template like this:

<div><span id="{{id}}">{{content}}</span></div>.

It's the same behavior as in custom tags as there could be plenty of HTML tags in custom style template - we cannot impose where it should be placed and we just don't know it - the end-user should do it himself.

In the example above we have two tags: div and span - we are unsure where the anchor should go, that's why we expose the {{id}} parameter to the template.

The same problem occurs with the {{align}} parameter. We cannot apply it to all the HTML tags in the template for the same reason as above - the end-user should directly place it himself.

@lserwatka lserwatka merged commit e10fecc into 1.1 Oct 7, 2020
@lserwatka lserwatka deleted the ezp-31231-custom-tag-attributes branch October 7, 2020 11:03
@lserwatka
Copy link
Member

You can merge it up.

@barw4
Copy link
Member Author

barw4 commented Oct 7, 2020

Merged into 2.1: 365e07b
Merged into master: 4e1c61f

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working QA approved
Development

Successfully merging this pull request may close these issues.

9 participants