Skip to content

[WIP] #558 #7 Developer can style content types output differently per viewport - fix broken tests #627

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

Closed

Conversation

lbvaimo
Copy link

@lbvaimo lbvaimo commented Sep 11, 2020

Story

As a developer I want to easily create themes and customizations to existing Page Builder content types to enable me align their visual output with my desired theme

Business Value

ability to style content with CSS classes not using !important

Acceptance Criteria

  • Developers are no longer required to use !important to style CSS properties

Technical Vision

  • Remove the usage of style attributes
  • Utilize style block generation and include a single block per instance
  • Consider including an optimizer to further decrease the size of the payload: https://github.com/css/csso

Story

#558

Related Pull Requests

https://github.com/magento/magento2-page-builder-ee/pull/173

Checklist

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All new or changed code is covered with unit/integration tests (if applicable)
  • All automated tests passed successfully (all builds are green)

Lukasz Borowiec added 4 commits September 10, 2020 15:17
#7 - Added ValidateContentTypeStyles action group
#7 - Added ValidateContentTypeStyles action group
#7 - Added ValidateContentTypeStyles action group
…eBuilderCatalogProductAddAndEditRowColumnSlideTest test
@lbvaimo lbvaimo changed the base branch from develop to csso-alternative September 11, 2020 11:50
@lbvaimo lbvaimo changed the title 558 7 content type style attribute removal [Story] Developer can style content types output differently per viewport #7 fix broken tests Sep 11, 2020
@lbvaimo lbvaimo linked an issue Sep 11, 2020 that may be closed by this pull request
7 tasks
@lbvaimo lbvaimo changed the title [Story] Developer can style content types output differently per viewport #7 fix broken tests #558 Developer can style content types output differently per viewport #7 fix broken tests Sep 11, 2020
@lbvaimo lbvaimo changed the title #558 Developer can style content types output differently per viewport #7 fix broken tests #558 [Story] #7 Developer can style content types output differently per viewport - fix broken tests Sep 11, 2020
@lbvaimo lbvaimo changed the title #558 [Story] #7 Developer can style content types output differently per viewport - fix broken tests [WIP] #558 #7 Developer can style content types output differently per viewport - fix broken tests Sep 11, 2020
@m2-community-project
Copy link

@lbvaimo unfortunately, only members of the maintainers team are allowed to assign developers to the pull request

@m2-community-project
Copy link

@lbvaimo unfortunately, only members of the maintainers team are allowed to assign developers to the pull request

1 similar comment
@m2-community-project
Copy link

@lbvaimo unfortunately, only members of the maintainers team are allowed to assign developers to the pull request

-->
<actionGroups xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xsi:noNamespaceSchemaLocation="urn:magento:mftf:Test/etc/actionGroupSchema.xsd">
<actionGroup name="validateAdvancedConfigurationAllOptions">
Copy link
Contributor

Choose a reason for hiding this comment

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

need to update ALL new actiongroups to adhere to the MFTF static checks

  • pascal case
  • name must end in ActionGroup
  • add description to each new action group

Copy link
Contributor

Choose a reason for hiding this comment

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

I made all changes

@@ -7,7 +7,7 @@
-->
<actionGroups xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xsi:noNamespaceSchemaLocation="urn:magento:mftf:Test/etc/actionGroupSchema.xsd">
<actionGroup name="validateAdvancedStyleWithAlignment">
<actionGroup name="validateAdvancedStyleWithAlignment" deprecated="Styles are not inline">
Copy link
Contributor

Choose a reason for hiding this comment

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

include the name of the actiongroup that external test writers should change to. for this one, that would be validateAdvancedConfigurationWithAlignment. so change to
deprecated="Styles are not inline. Use validateAdvancedConfigurationWithAlignment instead."

update ALL deprecated action groups

Copy link
Contributor

Choose a reason for hiding this comment

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

All deprecated action groups updated

-->
<actionGroups xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xsi:noNamespaceSchemaLocation="urn:magento:mftf:Test/etc/actionGroupSchema.xsd">
<actionGroup name="validateAdvancedConfigurationWithAlignment">
Copy link
Contributor

Choose a reason for hiding this comment

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

rather than rewriting the actiongroup, wouldn't it be better to extend validateAdvancedConfigurationAllOptions instead?

extend for all similar actiongroups below (advanced config, background config, heading config, etc)

Copy link
Contributor

Choose a reason for hiding this comment

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

completed

*/
-->
<actionGroups xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:noNamespaceSchemaLocation="urn:magento:mftf:Test/etc/actionGroupSchema.xsd">
<actionGroup name="validateBackgroundConfigurationWithNoImage">
Copy link
Contributor

Choose a reason for hiding this comment

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

rather than rewriting the actiongroup, wouldn't it be better to extend validateAllBackgroundAttributesActionGroup instead?

Copy link
Contributor

Choose a reason for hiding this comment

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

completed

<arguments>
<argument name="page" defaultValue=""/>
<argument name="index" defaultValue="1" type="string"/>
<argument name="lineColor" defaultValue="PageBuilderDividerLineColor_Default"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

these 3 data entity arguments would be much better if you change the arguments to be {{PageBuilderDividerLineColor_Default.rgb}} and line 21 to be {{lineColor}}. that way, test writers are not restricted to only using a data entity with an rgb value
also change lineThickness & lineWidth

Copy link
Contributor

Choose a reason for hiding this comment

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

completed

<actionGroup name="validateAdvancedConfigurationAllOptions">
<arguments>
<argument name="page" defaultValue=""/>
<argument name="alignment" defaultValue="PageBuilderAdvancedAlignmentPropertyDefault"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

change all new actiongroups like this one to use string arguments instead of data entity argument.

ex: change default value to {{PageBuilderAdvancedAlignmentPropertyDefault.value}} and line 26 to be {{alignment}}. that way, test writers are not restricted to only using a data entity with a specific key. (it's easy to see the value in this change for borderColor where you can have an rgb input, hex, rgba, plaintext, etc)

Copy link
Contributor

Choose a reason for hiding this comment

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

completed

Copy link
Contributor

Choose a reason for hiding this comment

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

@dhaecker

The drawback is that all modified tests will have to be changed again.
Rgb and other data keys (mainly rgb and value were used) can be always added to existing colors,
keeping everything as variables forces to use them, instead of unorganized string values
ex. rgb(250, 250, 250) as an argument instead of {{PageBuilderStageBackgroundColor_Default.rgb}}

<argument name="showControls" defaultValue="PageBuilderMapShowControls_Default"/>
<argument name="index" defaultValue="1" type="string"/>
</arguments>
<comment userInput="validateMapSettings" stepKey="comment"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

delete all <comment> actions on ALL new actiongroups

Copy link
Contributor

Choose a reason for hiding this comment

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

completed

<argument name="index" defaultValue="1" type="string"/>
</arguments>
<comment userInput="validateMapSettings" stepKey="comment"/>
<waitForElementVisible selector="{{page.base(index)}}" stepKey="seeMap"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

stepKey should be waitForMap

Copy link
Contributor

Choose a reason for hiding this comment

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

completed

</assertEquals>
<waitForElement selector="{{page.showControls(index, showControls.value)}}" stepKey="waitForShowControls"/>
<dontSeeElement selector="{{page.showControls(index, showControls.value)}}" stepKey="dontSeeShowControls"/>
<seeElementInDOM selector="{{page.showControls(index, showControls.value)}}" stepKey="seeInDOMShowControls"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure why the original actiongroup contained this line but it's useless. please delete

@@ -77,6 +77,31 @@
<entity name="PageBuilderButtonItemType_Link" type="pagebuilder_button_item_type_property" extends="PageBuilderButtonItemType_Template">
<data key="value">pagebuilder-button-link</data>
</entity>
<!-- Button Primary -->
<entity name="PageBuilderButtonPrimary_BackgroundColor" type="pagebuilder_button_item_primary_property">
Copy link
Contributor

Choose a reason for hiding this comment

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

why not throw all of the below into 1 data entity? something like PageBuilderButtonPrimary_StyleAttributes

dhaecker and others added 7 commits October 5, 2020 12:07
…onents are always nested in a Contained component

 - Fixing MFTF CR feedback
…onents are always nested in a Contained component

 - Fixing filename
…gento2-page-builder-631

[Imported] MC-35402: [Page Builder] New top-level Full-Width and Full-Bleed comp�
…ew groups, changed name in Pascal caps format
@petrovsa
Copy link
Contributor

petrovsa commented Oct 8, 2020

@magento run Functional Tests EE

@petrovsa
Copy link
Contributor

petrovsa commented Oct 8, 2020

@magento run Functional Tests EE

1 similar comment
@petrovsa
Copy link
Contributor

petrovsa commented Oct 9, 2020

@magento run Functional Tests EE

 - fixed bugs in test generation and changed all references on groups
@hannahnida
Copy link
Contributor

@magento run Functional Tests EE

1 similar comment
@petrovsa
Copy link
Contributor

@magento run Functional Tests EE

 - I changed references in PB-EE in PR 173
 - I made refactoring follow groups:
 ValidateAdvancedConfigurationWithAlignmentActionGroup
 ValidateAdvancedConfigurationWithBorderColorActionGroup
 ValidateAdvancedConfigurationWithCssClassesActionGroup
 ValidateAdvancedConfigurationWithNoAlignmentActionGroup
 ValidateAdvancedConfigurationWithNoAlignmentEmptyBorderRadiusActionGroup
 ValidateAdvancedConfigurationWithNoBorderActionGroup
 ValidateAdvancedConfigurationWithNoBorderRadiusActionGroup
@petrovsa
Copy link
Contributor

@magento run Functional Tests EE

…magento2-page-builder into 558_7_content-type-style-attribute-removal
@hannahnida
Copy link
Contributor

@magento run Functional Tests EE

hannahnida and others added 3 commits October 12, 2020 17:45
 - I made refactoring follow groups:
 ValidateAdvancedConfigurationWithNoBorderWidthActionGroup
 ValidateAdvancedConfigurationWithNoMarginsActionGroup
 ValidateAdvancedConfigurationWithNoPaddingActionGroup
…te-removal' into 558_7_content-type-style-attribute-removal
@petrovsa
Copy link
Contributor

@magento run Functional Tests EE

 - I made refactoring follow groups:
 ValidateAdvancedConfigurationWithNoMarginsActionGroup
 ValidateBackgroundConfigurationWithNoImageActionGroup
@petrovsa
Copy link
Contributor

@magento run Functional Tests EE

@omiroshnichenko omiroshnichenko deleted the 558_7_content-type-style-attribute-removal branch December 7, 2020 18:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Developer can style content types output differently per viewport
8 participants