Skip to content

Conversation

@HosamHasanRamadan
Copy link
Contributor

@HosamHasanRamadan HosamHasanRamadan commented Jul 9, 2024

fixes: flutter/flutter#150053
related to : flutter/flutter#96112

Pre-launch Checklist

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

@domesticmouse
Copy link
Contributor

domesticmouse commented Jul 11, 2024

I'll defer to @stuartmorgan if this is the best approach.

@stuartmorgan-g
Copy link
Collaborator

I'll defer to @stuartmorgan if this is the best approach.

I'm not familiar with this portion of the framework, so I'm not a good authority for this. @HosamHasanRamadan I would suggest asking for feedback on this in the #hackers-framework Discord channel.

@stuartmorgan-g

This comment was marked as resolved.

@HosamHasanRamadan

This comment was marked as resolved.

Copy link
Contributor

@nate-thegrate nate-thegrate left a comment

Choose a reason for hiding this comment

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

IMO, the ideal long-term solution would be to rework this API to use WidgetSpan objects for code snippets, to give full control over the background's padding and border radius.


If that's a bit too much work for your schedule, I think a less disruptive workaround would be to make the following change:

      code: theme.textTheme.bodyMedium!.copyWith(
-       backgroundColor: theme.cardTheme.color ?? theme.cardColor,
+       backgroundColor: theme.cardTheme.color,
        fontFamily: 'monospace',
        fontSize: theme.textTheme.bodyMedium!.fontSize! * 0.85,
      ),

We're already planning to get rid of that field at some point (see flutter/flutter#91772), so I'd be down with allowing selection highlight visibility by preemptively removing the theme.cardColor reference here.

@domesticmouse
Copy link
Contributor

LGTM once @nate-thegrate's request is integrated.

@HosamHasanRamadan
Copy link
Contributor Author

@nate-thegrate Ok , I will update the PR.
Once I have free time I will create new PR using widget span for code blocks

@HosamHasanRamadan
Copy link
Contributor Author

@nate-thegrate What about Cupertino theme color ?

code: theme.textTheme.textStyle.copyWith(

@nate-thegrate
Copy link
Contributor

@nate-thegrate What about Cupertino theme color ?

code: theme.textTheme.textStyle.copyWith(

It looks like the MarkdownStyleSheet.fromCupertinoTheme() constructor doesn't pull anything from the Cupertino theme for the background color, so I believe no change is necessary 👍

Copy link
Contributor

@nate-thegrate nate-thegrate left a comment

Choose a reason for hiding this comment

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

It looks like Linux repo_checks is failing since this PR currently doesn't include a changelog update.

I imagine we'll be good to land this change once that's addressed :)

Copy link
Contributor

@nate-thegrate nate-thegrate left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the contribution!

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.

Copy link
Collaborator

@stuartmorgan-g stuartmorgan-g left a comment

Choose a reason for hiding this comment

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

LGTM

@stuartmorgan-g stuartmorgan-g added the autosubmit Merge PR when tree becomes green via auto submit App label Sep 26, 2024
@auto-submit auto-submit bot merged commit c154d1d into flutter:main Sep 26, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 27, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 27, 2024
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Sep 27, 2024
flutter/packages@f38b780...0321757

2024-09-26 [email protected] Roll Flutter from 538e742 to fa402c8 (19 revisions) (flutter/packages#7713)
2024-09-26 [email protected] [flutter_markdown] remove backgroundColor from code text theme to enable code selection highlight (flutter/packages#7090)
2024-09-26 [email protected] [interactive_media_ads] Adds internal wrapper for iOS native `IMAFriendlyObstruction` (flutter/packages#7696)
2024-09-26 [email protected] Add a `@SuppressWarnings` in advance  (flutter/packages#7712)

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] 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
thejitenpatel pushed a commit to thejitenpatel/flutter that referenced this pull request Oct 1, 2024
flutter/packages@f38b780...0321757

2024-09-26 [email protected] Roll Flutter from 538e742 to fa402c8 (19 revisions) (flutter/packages#7713)
2024-09-26 [email protected] [flutter_markdown] remove backgroundColor from code text theme to enable code selection highlight (flutter/packages#7090)
2024-09-26 [email protected] [interactive_media_ads] Adds internal wrapper for iOS native `IMAFriendlyObstruction` (flutter/packages#7696)
2024-09-26 [email protected] Add a `@SuppressWarnings` in advance  (flutter/packages#7712)

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] 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
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.

[flutter_markdown] Text in code blocks has no visual indicator for selection

4 participants