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.

Sign up for GitHub

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

Jump to bottom

Add list start, reversed settings (in progress) #15113

Merged
merged 5 commits into from
Aug 15, 2019
Merged

Add list start, reversed settings (in progress) #15113

merged 5 commits into from
Aug 15, 2019

Conversation

Jackie6
Copy link
Contributor

@Jackie6 Jackie6 commented Apr 22, 2019 •
edited by ellatrix
Loading

Description

Partially fixes #13888
Fix #17040

How has this been tested?

  • unit test
  • browser test
  • e2e test but sometimes failed

Screenshots

YXv6W6sBZb

To do list

  • Add type start reversed options
  • Update the CSS style
  • Enable the null input for start value
  • Fix the preview inconsistent issue
  • Fix failed tests

@Jackie6 Jackie6 changed the title Add list type, start, reversed settings Add list type, start, reversed settings (in progress) Apr 22, 2019
ellatrix
ellatrix reviewed Apr 23, 2019
View reviewed changes
.travis.yml Outdated Show resolved Hide resolved
@ellatrix
Copy link
Member

Great work so far! Thank you!

It looks like when I set the list to reversed, the other attributes are also set:

Screenshot 2019-04-24 at 11 57 42

Also when I later set the list back to unordered (ul), the attributes remain:

Screenshot 2019-04-24 at 11 59 09

The should probably be reset to undefined.

@ellatrix ellatrix added [Block] List Affects the List Block [Type] Enhancement A suggestion for improvement. labels Apr 24, 2019
gziolo
gziolo reviewed Apr 25, 2019
View reviewed changes
packages/block-library/src/list/list-type-picker.js Outdated Show resolved Hide resolved
packages/block-library/src/list/editor.scss Outdated Show resolved Hide resolved
packages/block-editor/src/components/rich-text/editable.js Outdated Show resolved Hide resolved
packages/block-editor/src/components/rich-text/editable.js Outdated
@@ -119,6 +119,18 @@ export default class Editable extends Component {
this.editorNode.className = classnames( nextProps.className, CLASS_NAME );
}

if ( this.props.start !== nextProps.start ) {
this.editorNode.setAttribute( 'start', nextProps.start );
Copy link
Member

Choose a reason for hiding this comment

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

Why this node gets updated using setAttribute but in other places the value is set through direct mutation?

Copy link
Member

Choose a reason for hiding this comment

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

Probably because the others need to be removed if needed. Would be good to unify this though.

@ellatrix ellatrix added the [Status] In Progress Tracking issues with work in progress label Apr 25, 2019
@Jackie6 Jackie6 force-pushed the update/list-block branch from 3aae7fc to 88dc84c Compare April 25, 2019 18:06
ellatrix
ellatrix reviewed Apr 25, 2019
View reviewed changes
packages/block-library/src/list/edit.js Outdated
label={ __( 'List Type' ) }
value={ type }
options={ [
{ label: 'Decimal', value: '1' },
Copy link
Member

Choose a reason for hiding this comment

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

Would it be possible to unset the attribute if "Decimal" is selected?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why do we need to unset the attribute?

Copy link
Member

Choose a reason for hiding this comment

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

Thinking about it more, you are right that it shouldn't be unset. But then there should be one extra option that wouldn't set the attribute. Perhaps we can call this option "Theme", as it would have whatever type the theme defined (usually decimal).

Copy link
Member

Choose a reason for hiding this comment

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

@Jackie6 Could you add an extra button then to unset the value?

@Jackie6 Jackie6 requested a review from SergioEstevao as a code owner August 15, 2019 10:05
@ellatrix
Squash for easier merge:
3fbf11f 
f6ad4f07e6a1c8d99e55aa7acb01cf8ef1dfddd7 (HEAD -> update/list-block, Jackie6/update/list-block) Add id to BaseControl
212cab106044c4d7300d9441e48241b911db0bcb Fix unit test and e2e test
421a198bf58431c91b4c09d6ee45ba932762cb15 Refine the type definition of start and reversed
4172707059558ba9fc47d9557457e0b8e441f050 Fix typo
21ed1dbeb7881a9a5a6eea85fb3293e9d6ba0d5a Update CSS styles
925f1ab1ed073b31358a129c62abd56021e14c38 Update list type style and remove description
ef0aca95cb88203ceff78b81473e37ad8fc22714 Allow the start to be NaN
495166d3fc14591ceb88597915a19e976b2ebbeb Change to check strict equality
216ca51cfacfc174ca00d9669b88b53eaa6d0356 Fix the accidental change of travis yml
8e31259624ece930c385bf9cb7a28046505c7aff Change local state to attributes
09199ff9516aab099e47ed6e61fd4e2fadfdb95b Add list type, start, reversed settings
@ellatrix ellatrix changed the title Add list type, start, reversed settings (in progress) Add list start, reversed settings (in progress) Aug 15, 2019
@ellatrix ellatrix removed the [Status] In Progress Tracking issues with work in progress label Aug 15, 2019
@ellatrix ellatrix added this to the Gutenberg 6.4 milestone Aug 15, 2019
@ellatrix
Copy link
Member

Thank you, @Jackie6, for working on this PR! 🎉

@ellatrix ellatrix merged commit 43250ce into WordPress:master Aug 15, 2019
@ellatrix
Copy link
Member

@Jackie6 Feel free to create a separate PR for the type attribute. I would just implement with all the CSS overrides and push the (core) themes to change their CSS. Perhaps that should be done first. #15113 (comment)

@ellatrix ellatrix mentioned this pull request Aug 15, 2019
Closed
mchowning added a commit that referenced this pull request Aug 16, 2019
@mchowning
[RNMobile] update mobile to not use ListEdit
2c503a6 
This update is in response to changes made on the web side in #15113
that were causing a crash on mobile.
@mchowning mchowning mentioned this pull request Aug 16, 2019
Merged
5 tasks
mchowning added a commit that referenced this pull request Aug 18, 2019
@mchowning
Pass non shared logic into component with shared logic
c1f580e 
This update is in response to changes made on the web side in #15113
that were causing a crash on mobile.
mchowning added a commit that referenced this pull request Aug 20, 2019
@mchowning
[RNMobile] update mobile to not use ListEdit (#17070)
db70326 
* [RNMobile] update mobile to not use ListEdit

This update is in response to changes made on the web side in #15113
that were causing a crash on mobile.

* Extract list block logic not shared with mobile to separate component

* Improve name of AdditionalSettings component
@mchowning mchowning mentioned this pull request Aug 20, 2019
Closed
@hacknug hacknug mentioned this pull request Aug 21, 2019
Closed
@ellatrix ellatrix mentioned this pull request Aug 22, 2019
Merged
5 tasks
donmhico pushed a commit to donmhico/gutenberg that referenced this pull request Aug 27, 2019
@mchowning @donmhico
[RNMobile] update mobile to not use ListEdit (WordPress#17070)
3bcf6aa 
* [RNMobile] update mobile to not use ListEdit

This update is in response to changes made on the web side in WordPress#15113
that were causing a crash on mobile.

* Extract list block logic not shared with mobile to separate component

* Improve name of AdditionalSettings component
gziolo pushed a commit that referenced this pull request Aug 29, 2019
@Jackie6 @gziolo
List block: add start and reversed attributes (#15113)
62eba43 
* Squash for easier merge:
f6ad4f07e6a1c8d99e55aa7acb01cf8ef1dfddd7 (HEAD -> update/list-block, Jackie6/update/list-block) Add id to BaseControl
212cab106044c4d7300d9441e48241b911db0bcb Fix unit test and e2e test
421a198bf58431c91b4c09d6ee45ba932762cb15 Refine the type definition of start and reversed
4172707059558ba9fc47d9557457e0b8e441f050 Fix typo
21ed1dbeb7881a9a5a6eea85fb3293e9d6ba0d5a Update CSS styles
925f1ab1ed073b31358a129c62abd56021e14c38 Update list type style and remove description
ef0aca95cb88203ceff78b81473e37ad8fc22714 Allow the start to be NaN
495166d3fc14591ceb88597915a19e976b2ebbeb Change to check strict equality
216ca51cfacfc174ca00d9669b88b53eaa6d0356 Fix the accidental change of travis yml
8e31259624ece930c385bf9cb7a28046505c7aff Change local state to attributes
09199ff9516aab099e47ed6e61fd4e2fadfdb95b Add list type, start, reversed settings

* Revert adding  attribute for now, let's extract to a separate PR

* Use proper TextControl

* Adjust toggle control to unset attribute

* Clearer label
gziolo pushed a commit that referenced this pull request Aug 29, 2019
@mchowning @gziolo
[RNMobile] update mobile to not use ListEdit (#17070)
c674c37 
* [RNMobile] update mobile to not use ListEdit

This update is in response to changes made on the web side in #15113
that were causing a crash on mobile.

* Extract list block logic not shared with mobile to separate component

* Improve name of AdditionalSettings component
gziolo pushed a commit that referenced this pull request Aug 29, 2019
@Jackie6 @gziolo
List block: add start and reversed attributes (#15113)
9abda0e 
* Squash for easier merge:
f6ad4f07e6a1c8d99e55aa7acb01cf8ef1dfddd7 (HEAD -> update/list-block, Jackie6/update/list-block) Add id to BaseControl
212cab106044c4d7300d9441e48241b911db0bcb Fix unit test and e2e test
421a198bf58431c91b4c09d6ee45ba932762cb15 Refine the type definition of start and reversed
4172707059558ba9fc47d9557457e0b8e441f050 Fix typo
21ed1dbeb7881a9a5a6eea85fb3293e9d6ba0d5a Update CSS styles
925f1ab1ed073b31358a129c62abd56021e14c38 Update list type style and remove description
ef0aca95cb88203ceff78b81473e37ad8fc22714 Allow the start to be NaN
495166d3fc14591ceb88597915a19e976b2ebbeb Change to check strict equality
216ca51cfacfc174ca00d9669b88b53eaa6d0356 Fix the accidental change of travis yml
8e31259624ece930c385bf9cb7a28046505c7aff Change local state to attributes
09199ff9516aab099e47ed6e61fd4e2fadfdb95b Add list type, start, reversed settings

* Revert adding  attribute for now, let's extract to a separate PR

* Use proper TextControl

* Adjust toggle control to unset attribute

* Clearer label
gziolo pushed a commit that referenced this pull request Aug 29, 2019
@mchowning @gziolo
[RNMobile] update mobile to not use ListEdit (#17070)
169e87a 
* [RNMobile] update mobile to not use ListEdit

This update is in response to changes made on the web side in #15113
that were causing a crash on mobile.

* Extract list block logic not shared with mobile to separate component

* Improve name of AdditionalSettings component
@swissspidy swissspidy mentioned this pull request Sep 12, 2019
Closed
@jeffpaul
Copy link
Member

@Jackie6 I've tried to reach out via other channels, but I have been unsuccessful in getting in touch. I wanted to make sure you saw the request to you for consent to re-license your contributions to Gutenberg under GPLv2 and MPLv2 here: #31893. If you would kindly review that description and comment accordingly it would be greatly appreciated.

Thanks!
Jeff.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasmussen jasmussen jasmussen left review comments

@gziolo gziolo gziolo left review comments

@ellatrix ellatrix ellatrix approved these changes

@aduth aduth Awaiting requested review from aduth

@ajitbohra ajitbohra Awaiting requested review from ajitbohra

@chrisvanpatten chrisvanpatten Awaiting requested review from chrisvanpatten

@jorgefilipecosta jorgefilipecosta Awaiting requested review from jorgefilipecosta

@noisysocks noisysocks Awaiting requested review from noisysocks

@notnownikki notnownikki Awaiting requested review from notnownikki

@Soean Soean Awaiting requested review from Soean

@talldan talldan Awaiting requested review from talldan

@youknowriad youknowriad Awaiting requested review from youknowriad

@daniloercoli daniloercoli Awaiting requested review from daniloercoli

@etoledom etoledom Awaiting requested review from etoledom

@SergioEstevao SergioEstevao Awaiting requested review from SergioEstevao

Assignees
No one assigned
Labels
[Block] List Affects the List Block [Type] Enhancement A suggestion for improvement.
Projects
None yet
Milestone
Gutenberg 6.4
Development

Successfully merging this pull request may close these issues.

List block: allow dynamic numbering List Block: add type attribute for ordered list
5 participants
@Jackie6 @ellatrix @jeffpaul @gziolo @jasmussen