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

Remove editor dynamic width feature #1525

Merged
merged 3 commits into from
Nov 11, 2020
Merged

Conversation

Aljullu
Copy link
Contributor

@Aljullu Aljullu commented Nov 2, 2020

Closes #1512.

As discussed in #1520, we want to remove the feature to adapt the editor width to the selected template.

How to test the changes in this Pull Request:

  1. Create a page and switch between the 'Full width' template and the 'Default' template.
  2. Verify there are no visible changes in the editor, but when previewing the page in the frontend, the correct template is applied.
  3. Since this PR includes some changes to the built process, it might be good to test other JS features to ensure there are no regressions. For example, the sticky add to cart bar that can be added from the Customizer (under the WooCommerce > Product Page section) and appears in (not-external) product pages when you scroll down:
    imatge

Affected flows & features:

  • Block editor shows a realistic preview of the front end styling.

Changelog

Removal – Removed the feature to adapt the editor width to the selected template.

@Aljullu Aljullu added status: needs review PR that needs review editor Issues related to the editor styles labels Nov 2, 2020
@Aljullu Aljullu self-assigned this Nov 2, 2020
@Aljullu Aljullu requested review from haszari and nerrad November 2, 2020 14:01
@Aljullu
Copy link
Contributor Author

Aljullu commented Nov 2, 2020

I didn't set a specific milestone because I don't think that needs to block 2.9.0 release.

Copy link
Member

@haszari haszari left a comment

Choose a reason for hiding this comment

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

Great to see the code base getting simpler (more robust). I tested editing a page and changing templates, and also tested most JS functionality (sticky add-to-cart, product pagination, mobile footer, mobile menu) – everything worked well in my testing (Chrome / Safari).

Overall this is looking splendid 👌, but there are a couple of things to consider before merge:

  1. Whether we completely remove babel dependency, or keep it around. Up to you, I'm probably leaning towards remove. (See comment.)
  2. The content width in the editor defaults to the "wide" view. I think it should default to narrow, as this matches the default template (with sidebar widgets), and works better when editing on smaller screens/windows. (See screenshot below.)

Screen Shot 2020-11-03 at 4 27 26 PM

Pre-approving (those fixes should be straightforward), feel free to ping me for another review if you like ✋

package.json Outdated
@@ -34,7 +34,6 @@
"devDependencies": {
"@babel/core": "7.12.3",
Copy link
Member

Choose a reason for hiding this comment

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

Do we need babel dependencies any more (@babel/core grunt-babel)? I'm assuming that editor.js was the only file using ES6/babel-powered JS syntax - is that correct? Looks like babel is (mostly) removed from Gruntfile (in this PR).

There are two options:

  1. Keep babel installed and run the task, even if it's not needed. This is "future proof", we have babel running and can update existing code to use ES6 when fixing bugs.
  2. Completely remove babel entirely, since it's not strictly needed right now.

@Aljullu Up to you which approach we take here. From a quick look at the gruntfile, I think we need to remove grunt.loadNpmTasks( 'grunt-babel' ); as well (if you decide on option 2).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point! Removing them sounds like the best option to me, it makes the codebase simpler and decreases the maintenance burden. I have removed babel completely in 9dec992, we can always install it back if we need it in the future.

@Aljullu Aljullu force-pushed the remove/dynamic-editor-width branch from 16db3d8 to 26f80a9 Compare November 9, 2020 14:17
@Aljullu Aljullu added this to the 3.0.0 milestone Nov 9, 2020
@Aljullu
Copy link
Contributor Author

Aljullu commented Nov 9, 2020

Thanks for the review @haszari!

The content width in the editor defaults to the "wide" view.

Good catch! It definitely makes more sense defaulting to the narrow width. Should be fixed in 26f80a9.

I will wait merging this PR until tomorrow, so it targets Storefront 3.0 instead of 2.9, which is to be released tomorrow.

@Aljullu Aljullu force-pushed the remove/dynamic-editor-width branch from 26f80a9 to 259c715 Compare November 11, 2020 08:05
@Aljullu Aljullu requested a review from a team as a code owner November 11, 2020 08:05
@Aljullu Aljullu merged commit d867a14 into trunk Nov 11, 2020
@Aljullu Aljullu deleted the remove/dynamic-editor-width branch November 11, 2020 08:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
editor Issues related to the editor styles status: needs review PR that needs review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

WordPress 5.6 compatibility – editor width doesn't change based on full-width/default template
2 participants