-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
<ul type> & <ol type> #11699
<ul type> & <ol type> #11699
Conversation
* Converts `list-style-type` style to `type` attribute of `<ul>` or `<ol>` elements. | ||
* | ||
* @param {String} value | ||
* @retun {String|null} |
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.
return
* Converts `type` attribute of `<ul>` or `<ol>` elements to `list-style-type` equivalent. | ||
* | ||
* @param {String} value | ||
* @retun {String|null} |
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.
return
writer.removeStyle( 'list-style-type', element ); | ||
} | ||
}, | ||
setAttributeOnDowncast: useAttribute ? |
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.
There's nothing wrong with keeping this logic within a single function. Using tenary operator like this only makes it harder to read.
@@ -68,7 +68,7 @@ export default class ListProperties extends Plugin { | |||
* When set, the list style feature will be enabled. It allows changing the `list-style-type` HTML attribute of the lists. |
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.
It's worth mentioning here that it's controlling either list-style-type
style or the type
attribute depending on the value.
@@ -277,7 +277,12 @@ function createListPropertiesView( { | |||
listStyleCommand | |||
} ); | |||
|
|||
styleButtonViews = styleDefinitions.map( styleButtonCreator ); | |||
// The command can be ListStyleCommand or DocumentListStyleComman. Let's checks if it has expected function. |
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.
DocumentListStyleCommand
@@ -87,7 +87,8 @@ | |||
], | |||
"attributes": [ | |||
"start", | |||
"reversed" | |||
"reversed", | |||
"type" |
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.
Are you sure this is OK? Only the DocumentListProperties
supports the type
attribute. ListProperties
is incapable of returning type
in the data pipeline.
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.
LGTM. Minor changes requested.
…e in ListPropertiesUI because it causes problems in tests.
YAY! This unblocks https://www.drupal.org/project/drupal/issues/3274635 — which means Drupal will be able to offer the superior list type UX that CKE5 supports, instead of the GHS-based work-around we were planning to ship with in https://www.drupal.org/project/drupal/issues/3274651 👍 🥳 |
Suggested merge commit message (convention)
Feature (list): Adds support for
type
attribute of<ul>
and<ol>
elements in addition tolist-style-type
style. Closes #11615.Additional information
For example – encountered issues, assumptions you had to make, other affected tickets, etc.