-
Notifications
You must be signed in to change notification settings - Fork 472
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
Conversation
I didn't set a specific milestone because I don't think that needs to block 2.9.0 release. |
There was a problem hiding this 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:
- Whether we completely remove
babel
dependency, or keep it around. Up to you, I'm probably leaning towards remove. (See comment.) - 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.)
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", |
There was a problem hiding this comment.
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:
- 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.
- 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).
There was a problem hiding this comment.
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.
16db3d8
to
26f80a9
Compare
Thanks for the review @haszari!
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. |
26f80a9
to
259c715
Compare
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:
Affected flows & features:
Changelog