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

Support alternate bullet style per level #1354

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from

Conversation

cbess
Copy link
Contributor

@cbess cbess commented Mar 29, 2022

Similar to web browsers, draw different bullets at different levels.

To test:

  • Create an unordered list
  • Add multiple levels to it
  • Notice the bullet style for level one is different than level two and three
Before After
old new

similar to web browsers, draw different bullets at different levels
@cbess
Copy link
Contributor Author

cbess commented Jul 29, 2022

@crazytonyli can this be merged?

@diegoreymendez diegoreymendez self-requested a review July 29, 2022 09:13
commit 4d0522d
Merge: 2b54f87 59d5090
Author: Gio Lodi <[email protected]>
Date:   Thu Mar 9 12:08:36 2023 +1100

    Point `bash-cache` plugin to new `a8c-ci-toolkit` location (wordpress-mobile#1367)

commit 59d5090
Author: Gio Lodi <[email protected]>
Date:   Mon Mar 6 21:10:15 2023 +1100

    Remove unused YAML anchor and fix indentation in pipeline

commit e0526bf
Author: Gio Lodi <[email protected]>
Date:   Mon Mar 6 21:09:53 2023 +1100

    Point `bash-cache` plugin to new `a8c-ci-toolkit` location

    While I was at it, I also updated the version to the latest, 2.13.0.

    This was done via:

    ```
    find . -type f -name "*.yml" -exec sed -i '' 's/automattic\/bash-cache#[0-9.]\{1,\}/automattic\/a8c-ci-toolkit#2.13.0/g' {} +
    ```

commit 2b54f87
Merge: b7d8a70 d18d0a8
Author: Gio Lodi <[email protected]>
Date:   Thu Oct 20 16:59:18 2022 +1100

    Add `CHANGELOG.md` (wordpress-mobile#1365)

commit d18d0a8
Author: Gio Lodi <[email protected]>
Date:   Thu Oct 20 15:18:16 2022 +1100

    Update `PULL_REQUEST_TEMPLATE.md` with note about `CHANGELOG.md`

commit 2c73ef5
Author: Gio Lodi <[email protected]>
Date:   Thu Oct 20 14:32:38 2022 +1100

    Add changelog entry about changelog itself

commit 212ce2f
Author: Gio Lodi <[email protected]>
Date:   Thu Oct 20 14:15:12 2022 +1100

    Update `CHANGELOG.md` with new format

commit b7d8a70
Merge: 91cf065 40afa55
Author: Gio Lodi <[email protected]>
Date:   Tue Oct 4 10:29:42 2022 +1100

    Tools upgrade: Ruby 2.7.4, bash-cache 2.8.0 (wordpress-mobile#1363)

commit 40afa55
Author: Gio Lodi <[email protected]>
Date:   Mon Oct 3 20:00:33 2022 +1100

    Use `bash-cache` Buildkite plugin version 2.8.0

    This version includes a fix that removes the need to run
    `gem install bundler` before each step.

commit bd8840a
Author: Gio Lodi <[email protected]>
Date:   Mon Oct 3 19:59:52 2022 +1100

    Use Ruby version 2.7.4

commit 91cf065
Merge: 9d8d9b8 1ce621c
Author: Diego Rey Mendez <[email protected]>
Date:   Wed Aug 10 15:07:41 2022 +0200

    Merge pull request wordpress-mobile#1353 from cbess/expose-indentation-actions

    Expose indentation operations

commit 9d8d9b8
Merge: b16c763 7eb60c7
Author: Tony Li <[email protected]>
Date:   Mon Jul 25 11:11:13 2022 +1200

    Merge pull request wordpress-mobile#1359 from wordpress-mobile/remove/circle-ci

    Remove CircleCI

commit 7eb60c7
Author: Jeremy Massel <[email protected]>
Date:   Fri Jul 22 23:23:27 2022 -0600

    Trigger Build

commit f91511e
Author: Jeremy Massel <[email protected]>
Date:   Thu Jul 21 23:03:28 2022 -0600

    Remove CircleCI

commit 1ce621c
Author: C. Bess <[email protected]>
Date:   Sat Mar 26 12:31:51 2022 -0500

    cleanup whitespace

commit ed55d4a
Author: C. Bess <[email protected]>
Date:   Sat Mar 26 12:31:03 2022 -0500

    cleanup whitespace

commit 4561534
Author: C. Bess <[email protected]>
Date:   Sat Mar 26 12:01:30 2022 -0500

    expose indentation operations

    - make increase/decrease indentation actions visible
    - cleanup code
- support indent style option
@cbess
Copy link
Contributor Author

cbess commented Aug 6, 2023

@diegoreymendez it is ready for review again

@diegoreymendez
Copy link
Contributor

@cbess - Unfortunately, after my departure from Automattic I'm no longer maintaining this project.

You may need to ping one of the other maintainers to get this reviewed and merged.

@cbess
Copy link
Contributor Author

cbess commented Aug 8, 2023

@mokagio I've made the necessary changes and its ready for review

@cbess
Copy link
Contributor Author

cbess commented Nov 11, 2023

@fluiddot this is connected to #1353
Can this be reviewed and merged?

@diegoreymendez
Copy link
Contributor

@jleandroperez, @frosty - Hey folks, any idea who could look at this one?

@jleandroperez
Copy link
Contributor

@twstokes Do you think it'd be possible to validate this changeset?

I'm afraid I haven't been involved with WPiOS / Jetpack in a long, long time, and we'd really need to ensure nothing else breaks.

Thanks in advance!!

P.s.: Hola @diegoreymendez !!!

@twstokes
Copy link
Contributor

twstokes commented Jan 8, 2024

@twstokes Do you think it'd be possible to validate this changeset?

👋 Howdy @jleandroperez, will be happy to give it a look. I have a slight backlog due to the new year, but will check this soon.

@jleandroperez
Copy link
Contributor

Thanks a million @twstokes !!!

@twstokes twstokes self-requested a review January 17, 2024 18:37
Copy link
Contributor

@twstokes twstokes left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution @cbess and sorry for the delays!

I have limited experience with this library's codebase, but conceptually what you've added makes sense to me and passed my manual tests. 🎉

I've added some comments that I think are optional, but could help make the changes more focused. Renaming IndentStyle to show it's the indentation style for unordered lists only could be an improvement. Making it an associated value for .unordered would be even better, but I understand that could add some complications to the Int raw type.

I'd also like to wait until this PR is finished (which is being actively worked on) so we can be sure of our unit tests.

markerNumber += start
let markerString = list.style.markerText(forItemNumber: markerNumber)
let markerString = list.style.markerText(forItemNumber: markerNumber, indentLevel: indentLevel)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: The indentLevel isn't accurate if the list style isn't unordered + standard, which could cause confusion.

}
}
}

/// List Indent Styles
///
public enum IndentStyle: Int {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Since this only applies to unordered lists, we should consider making this clearer.


// calculate current indent level
let indentWidth = paragraphStyle.indentToLast(TextList.self)
if let firstLevelWidth = firstLevelWidth {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: I'd consider refactoring this since we know that at this point firstLevelWidth should never be nil.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed

@@ -228,6 +228,17 @@ open class TextView: UITextView {

var maximumListIndentationLevels = 7

/// The list indent style
/// Default is `default`, single style for each level.
public var listIndentStyle: TextList.IndentStyle {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Since this only applies to unordered lists, we should consider making this clearer.

@@ -27,6 +27,9 @@ class LayoutManager: NSLayoutManager {
///
var blockquoteBorderWidth: CGFloat = 2

/// The list indent style
///
var listIndentStyle: TextList.IndentStyle = .default
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Since this only applies to unordered lists, we should consider making this clearer.

@cbess
Copy link
Contributor Author

cbess commented Jan 25, 2024

@twstokes I'll make those Nit: changes you mentioned

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.

4 participants