Skip to content

[flutter_markdown] Ensure customize nested bullet list style. #6384

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

Merged
merged 9 commits into from
Apr 19, 2024

Conversation

Kurogoma4D
Copy link
Contributor

@Kurogoma4D Kurogoma4D commented Mar 25, 2024

See flutter/flutter#145670 .

Pre-launch Checklist

If you need help, consider asking for advice on the #hackers-new channel on Discord.

Copy link
Contributor

@domesticmouse domesticmouse left a comment

Choose a reason for hiding this comment

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

@stuartmorgan-g
Copy link
Contributor

CI will not pass without completing these steps; that's why repo_checks is red.

@Kurogoma4D
Copy link
Contributor Author

I've updated them (I'm new to contribute this package, so I'm not sure this versioning is correct).

@tarrinneal
Copy link
Contributor

@domesticmouse This is a breaking change isn't it?

@domesticmouse
Copy link
Contributor

@domesticmouse This is a breaking change isn't it?

Do we follow SemVer in these packages? If we do, then yeah it is.

@Kurogoma4D
Copy link
Contributor Author

So should I increase the minor version?

@tarrinneal
Copy link
Contributor

@domesticmouse This is a breaking change isn't it?

Do we follow SemVer in these packages? If we do, then yeah it is.

We're generally stricter than that. No breaking changes, we would deprecate this method and add a new one. I'm not sure if that's the policy for this package. @stuartmorgan will know.

@stuartmorgan-g
Copy link
Contributor

We're generally stricter than that. No breaking changes, we would deprecate this method and add a new one. I'm not sure if that's the policy for this package.

That's for platform interface packages. For regular packages we can do breaking changes (with a corresponding major version bump), but we generally evaluate the importance of the use case and whether we need to do it with a breaking change or there is a less disruptive option.

@tarrinneal
Copy link
Contributor

That's for platform interface packages.

Right.

So should I increase the minor version?

@Kurogoma4D That should be fine. I think change is worth the breaking change.

You should also make a note in the changelog about what this pr breaks https://github.com/flutter/flutter/wiki/Contributing-to-Plugins-and-Packages#changelog

typedef MarkdownBulletBuilder = Widget Function(
int index,
BulletStyle style,
int nestLevel,
Copy link
Contributor

Choose a reason for hiding this comment

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

I haven't done a full review here, but I assume the breaking change is that we're adding a parameter to a method that package clients need to implement?

If so, it would be better to make the breaking change replace it with a parameter object, so that next time it doesn't need a breaking change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree.
Which is better to use class or record, as a parameter object here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Adding a field to a record later would be breaking (since it changes the type), so it needs to be a class to get the benefit.

@Kurogoma4D
Copy link
Contributor Author

@domesticmouse @stuartmorgan
Could you review again?

Copy link
Contributor

@domesticmouse domesticmouse left a comment

Choose a reason for hiding this comment

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

@domesticmouse
Copy link
Contributor

Kurogoma4D, I think I've resolved the conflicts appropriately, please check.

@tarrinneal @stuartmorgan PTAL

@Kurogoma4D
Copy link
Contributor Author

@domesticmouse Thanks, LGTM :)

@tarrinneal tarrinneal added the autosubmit Merge PR when tree becomes green via auto submit App label Apr 19, 2024
@auto-submit auto-submit bot merged commit 5d15437 into flutter:main Apr 19, 2024
78 checks passed
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Apr 19, 2024
flutter/packages@0e3809d...88a3a56

2024-04-19 [email protected] [camera] Convert iOS Obj-C->Dart calls to Pigeon (flutter/packages#6568)
2024-04-19 [email protected] [flutter_markdown] Ensure customize nested bullet list style. (flutter/packages#6384)
2024-04-18 [email protected] [ci] Add more dev dependency checks, and fix errors (flutter/packages#6563)
2024-04-18 [email protected] Roll Flutter from 3882afb to fb110b9 (56 revisions) (flutter/packages#6565)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages-flutter-autoroll
Please CC [email protected],[email protected] on the revert to ensure that a human
is aware of the problem.

To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
gilnobrega pushed a commit to gilnobrega/flutter that referenced this pull request Apr 22, 2024
flutter/packages@0e3809d...88a3a56

2024-04-19 [email protected] [camera] Convert iOS Obj-C->Dart calls to Pigeon (flutter/packages#6568)
2024-04-19 [email protected] [flutter_markdown] Ensure customize nested bullet list style. (flutter/packages#6384)
2024-04-18 [email protected] [ci] Add more dev dependency checks, and fix errors (flutter/packages#6563)
2024-04-18 [email protected] Roll Flutter from 3882afb to fb110b9 (56 revisions) (flutter/packages#6565)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages-flutter-autoroll
Please CC [email protected],[email protected] on the revert to ensure that a human
is aware of the problem.

To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
TecHaxter pushed a commit to TecHaxter/flutter_packages that referenced this pull request May 22, 2024
arc-yong pushed a commit to Arctuition/packages-arc that referenced this pull request Jun 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App p: flutter_markdown
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants