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

Developer can style content types output differently per viewport #2694

Merged
merged 6 commits into from
Nov 2, 2020

Conversation

omiroshnichenko
Copy link
Contributor

@omiroshnichenko omiroshnichenko commented Sep 10, 2020

❗❗DO NOT MERGE BEFORE magento/magento2-page-builder#558 closed ❗❗

Description

Convert style blocks to inline styles to prevent BIC changes

Related Issue

https://jira.corp.magento.com/browse/PB-558
magento/magento2-page-builder#558

Acceptance

Verification Stakeholders

Specification

Verification Steps

  1. Go to any page with Page Builder content.
  2. Verify that content renders as before.

Screenshots / Screen Captures (if appropriate)

Checklist

  • I have added tests to cover my changes, if necessary.
  • I have updated the documentation accordingly, if necessary.

…output differently per viewport

- convert style blocks to inline styles to prevent BIC changes
@PWAStudioBot
Copy link
Contributor

PWAStudioBot commented Sep 10, 2020

Messages
📖

Access a deployed version of this PR here. Make sure to wait for the "pwa-pull-request-deploy" job to complete.

📖 DangerCI Failures related to missing labels/description/linked issues/etc will persist until the next push or next nightly build run (assuming they are fixed).
📖

Associated JIRA tickets: PB-558.

Generated by 🚫 dangerJS against 9458cc4

…output differently per viewport

- fix code style
const selectors = rule.selectorText
.split(',')
.map(selector => selector.trim());
selectors.forEach(selector => {

Choose a reason for hiding this comment

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

Readability: New line after const and/or before forEach loop.

if (!styles[selector]) {
styles[selector] = [];
}
styles[selector].push(rule.style);

Choose a reason for hiding this comment

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

Readability: New line after if block.

@davemacaulay
Copy link
Contributor

The code looks great, and I appreciate the solution builds in backwards compatibility with older versions of Page Builder. How do we intend to handle media queries once they're introduced for responsive aspects of Page Builder? We can't just push these back into inline styles.

Performance is of huge importance for PWA Studio, could you run a performance analysis of old content which has the style tags inline and then new content which needs to be converted to help better understand the negative impact this has on performance.

@omiroshnichenko
Copy link
Contributor Author

@davemacaulay
Performance results performed on the page which contains all content types.
Before:
Screen Shot 2020-10-05 at 5 04 54 PM
After:
Screen Shot 2020-10-05 at 5 03 41 PM
Lighthouse results are the same.

Media queries and other styles could be stored in the registry and received to read in configAgregators. This is will be the next iteration of changes in the Page Builder PWA package.

@davemacaulay
Copy link
Contributor

@omiroshnichenko looks good, the 2ms increase in time for the parseStorageHtml function is acceptable in my eyes.

@davemacaulay davemacaulay added the version: Minor This changeset includes functionality added in a backwards compatible manner. label Oct 5, 2020
@bluemwhitew
Copy link

Added Manual Test to parent issue magento/magento2-page-builder#558.

@dpatil-magento
Copy link
Contributor

QA Approved.

@dpatil-magento dpatil-magento merged commit 890276b into magento:develop Nov 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg:pagebuilder Progress: done version: Minor This changeset includes functionality added in a backwards compatible manner.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants