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

[plg_editors_codemirror] Manifest cleanup #20544

Merged
merged 7 commits into from Dec 27, 2018
Merged

[plg_editors_codemirror] Manifest cleanup #20544

merged 7 commits into from Dec 27, 2018

Conversation

SharkyKZ
Copy link
Contributor

Pull Request for Issue # .

Summary of Changes

Added/replaced filters (options is not a valid filter, assume it was mistaken for validation rule).
Synced fallback values.
Removed doubled empty value option from appearance field.

Testing Instructions

  1. Code review.
  2. Check that editor options toggle on and off correctly.

Documentation Changes Required

No.

@Quy
Copy link
Contributor

Quy commented May 27, 2018

@@ -71,7 +71,7 @@
description="PLG_CODEMIRROR_FIELD_LINEWRAPPING_DESC"
Copy link
Contributor

Choose a reason for hiding this comment

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

type should be radio

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was set to hidden on purpose https://github.com/joomla/joomla-cms/blob/staging/plugins/editors/codemirror/codemirror.php#L210-L211.

But disabling line wrapping seems to work fine now.


// Add styling to the active line.
$options->styleActiveLine = (boolean) $this->params->get('activeLine', true);
$options->styleActiveLine = (boolean) $this->params->get('activeLine', 1);

// Add styling to the active line.
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment as above?

@Quy
Copy link
Contributor

Quy commented May 28, 2018

I have tested this item ✅ successfully on 24304b6


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/20544.

@Quy
Copy link
Contributor

Quy commented Jun 29, 2018

I have tested this item ✅ successfully on 46119f0


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/20544.

1 similar comment
@viocassel
Copy link
Contributor

I have tested this item ✅ successfully on 46119f0


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/20544.

@Quy
Copy link
Contributor

Quy commented Dec 24, 2018

RTC


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/20544.

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Dec 24, 2018
@@ -161,7 +161,6 @@
label="PLG_CODEMIRROR_FIELD_KEYMAP_LABEL"
description="PLG_CODEMIRROR_FIELD_KEYMAP_DESC"
default=""
filter="options"
Copy link
Contributor

Choose a reason for hiding this comment

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

The filter should remain here (it ensures that only the options listed can be saved, prevents potential DOM manipulation)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was mistaken for form rule. It should have been validate="options". Since filter="options" does nothing, I've removed it for now. Planning to add validation in separate PRs.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, you're right. Getting my wires crossed again.

@@ -175,7 +174,6 @@
label="PLG_CODEMIRROR_FIELD_FULLSCREEN_LABEL"
description="PLG_CODEMIRROR_FIELD_FULLSCREEN_DESC"
default="F10"
filter="options"
Copy link
Contributor

Choose a reason for hiding this comment

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

The filter should remain here (it ensures that only the options listed can be saved, prevents potential DOM manipulation)

@@ -196,7 +194,6 @@
type="checkboxes"
label="PLG_CODEMIRROR_FIELD_FULLSCREEN_MOD_LABEL"
description="PLG_CODEMIRROR_FIELD_FULLSCREEN_MOD_DESC"
filter="options"
Copy link
Contributor

Choose a reason for hiding this comment

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

The filter should remain here (it ensures that only the options listed can be saved, prevents potential DOM manipulation)

@mbabker mbabker merged commit 2c933f7 into joomla:staging Dec 27, 2018
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Dec 27, 2018
@mbabker mbabker added this to the Joomla 3.9.2 milestone Dec 27, 2018
@SharkyKZ SharkyKZ deleted the plg_editors_1 branch December 29, 2018 21:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants