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

Markdown block: remove module dependency #13473

Merged
merged 1 commit into from
Sep 19, 2019
Merged

Conversation

simison
Copy link
Member

@simison simison commented Sep 15, 2019

Currently, Markdown block requires the Markdown module to be present, although we don't rely on the module technically.

Markdown block renders Markdown using a frontend library instead of relying on the PHP module.

UI where you need to go to turn on a separate toggle in settings page just to allow a feature showing up in the block picker is not great. From the user perspective, the act of choosing Markdown block from the block picker is de-factor "turning a feature on".

Module toggle itself is still useful for allowing writing Markdown on classic editor pages and the toggle could be hidden for sites using Gutenberg, but that's outside the scope in here.

This PR removes module dependency, making the block always available in the block picker.

Similar question to Tiled gallery's Photon module dependency; #13471

Changes proposed in this Pull Request:

  • Remove markdown module check when registering markdown block

Is this a new feature or does it add/remove features to an existing part of Jetpack?

Modify existing.

Testing instructions:

Proposed changelog entry for your changes:

  • Show Markdown block in the block picker even if the Markdown module is disabled

@simison simison added [Feature] Markdown [Focus] Blocks Issues related to the block editor, aka Gutenberg, and its extensions developed in Jetpack labels Sep 15, 2019
@simison simison requested a review from a team September 15, 2019 15:38
@jetpackbot
Copy link

Warnings
⚠️

The PR is missing at least one [Status] label. Suggestions: [Status] In Progress, [Status] Needs Review

This is an automated check which relies on PULL_REQUEST_TEMPLATE. We encourage you to follow that template as it helps Jetpack maintainers do their job. If you think 'Testing instructions' or 'Proposed changelog entry' are not needed for your PR - please explain why you think so. Thanks for cooperation 🤖

Generated by 🚫 dangerJS against fa15e65

@matticbot
Copy link
Contributor

Caution: This PR has changes that must be merged to WordPress.com
Hello simison! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer, review, and approve D32834-code before merging this PR. Thank you!

Copy link
Member

@zinigor zinigor left a comment

Choose a reason for hiding this comment

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

Seems to work well, thanks for this! Feel free to label this one as Reviewed

Copy link
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

Was there some discussion about it somewhere? While this works well and makes sense, I wonder if this could create some confusion for folks who didn't turn on that feature and would not expect to see it appear in their block editor.

I am not against the idea, I think it's pretty nice, but I just want to be sure we all agree on that approach.

@jeherve jeherve added this to the 7.8 milestone Sep 17, 2019
@simison
Copy link
Member Author

simison commented Sep 17, 2019

Was there some discussion about it somewhere?

No recent new conversations no, this is just what I think makes sense in "fix the flows" context no matter the cons. ;-) I think at this point there are already so many conversations in that we're well aware of all cons/pros (happy to list them all here if you'd prefer) and just need a decision if this is ok direction.

While this works well and makes sense, I wonder if this could create some confusion for folks who didn't turn on that feature and would not expect to see it appear in their block editor.

Valid point! I think that's fine tho because the confusion other way around when feature isn't available without extra toggle is greater, considering how the block picker is fairly contained & hidden place for features to pop-up in. If that makes sense?

@jeherve
Copy link
Member

jeherve commented Sep 19, 2019

I'm happy to merge this, and since it's got 2 reviews already, I'll do just that.

@jeherve jeherve merged commit 8d31145 into master Sep 19, 2019
@jeherve jeherve deleted the update/markdown-module-dep branch September 19, 2019 07:22
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Sep 19, 2019
jeherve added a commit that referenced this pull request Sep 20, 2019
jeherve added a commit that referenced this pull request Sep 24, 2019
jeherve added a commit that referenced this pull request Sep 24, 2019
* Changelog: initial set of changes for 7.8

* Changelog: add #13310

* Changelog: add #13103

* Changelog: add #13426

* Changelog: add #13389

* Changelog: add #13449

* Changelog: add #13461

* Changelog: add #13460

* Changelog: add #13441

* Changelog: add #13454

* Changelog: add #13457

* Changelog: add #13425

* Changelog: add #13473

* Changelog: add #13355

* Changelog: add #13451

* Changelog: add #13358

* Changelog: add #13464

* Changelog: add #13416

* Changelog: add #13494

* Changelog: add #13465

* Changelog: add #13424

* Changelog: add #13432

* Changelog: add #13471

* Changelog: add 7.7.2 entry

* Changelog: add #13446

* Add more testing elements
jeherve added a commit that referenced this pull request Sep 24, 2019
* Changelog: initial set of changes for 7.8

* Changelog: add #13310

* Changelog: add #13103

* Changelog: add #13426

* Changelog: add #13389

* Changelog: add #13449

* Changelog: add #13461

* Changelog: add #13460

* Changelog: add #13441

* Changelog: add #13454

* Changelog: add #13457

* Changelog: add #13425

* Changelog: add #13473

* Changelog: add #13355

* Changelog: add #13451

* Changelog: add #13358

* Changelog: add #13464

* Changelog: add #13416

* Changelog: add #13494

* Changelog: add #13465

* Changelog: add #13424

* Changelog: add #13432

* Changelog: add #13471

* Changelog: add 7.7.2 entry

* Changelog: add #13446

* Add more testing elements
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Markdown [Focus] Blocks Issues related to the block editor, aka Gutenberg, and its extensions developed in Jetpack Touches WP.com Files
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants