-
Notifications
You must be signed in to change notification settings - Fork 281
[SuperEditor][SuperReader][SuperTextField] Merge stylesheet styles with default text style. (Resolves #2727) #2750
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
base: main
Are you sure you want to change the base?
Conversation
EDIT: I made it opt-in for SuperTextField also.
|
InlineWidgetBuilderChain inlineWidgetBuilders, | ||
) { | ||
InlineWidgetBuilderChain inlineWidgetBuilders, { | ||
bool inheritDefaultTextStyle = false, |
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.
I'm wondering if this is too low level of a location to instruct the method to inherit the default text style. I know we provide a BuildContext
but I'm wondering if it makes sense for this generic extension to have a concept of inheriting a text style. Would it make more sense for the caller to pass that in?
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 makes sense. Updated it.
); | ||
}); | ||
|
||
testWidgetsOnArbitraryDesktop('inherits the enclosing default text style if requested', (tester) async { |
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.
Can you add a test that rebuilds the tree with a different style and makes sure that the new value is used after the pump?
Please do the same for SuperReader
and SuperTextField
.
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.
I already added a "changes visual text when the enclosing default text style changes" to each of them.
SingleColumnStylesheetStyler({ | ||
required Stylesheet stylesheet, | ||
}) : _stylesheet = stylesheet; | ||
TextStyle? defaultTextStyle, |
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.
Please add a constructor Dart Doc that explains what should be passed for defaultTextStyle
and what happens if nothing is passed for it.
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.
Updated.
super_editor/lib/src/default_editor/layout_single_column/_styler_shylesheet.dart
Outdated
Show resolved
Hide resolved
super_editor/lib/src/default_editor/layout_single_column/_styler_shylesheet.dart
Outdated
Show resolved
Hide resolved
super_editor/lib/src/default_editor/layout_single_column/_styler_shylesheet.dart
Outdated
Show resolved
Hide resolved
super.didChangeDependencies(); | ||
|
||
if (_docLayoutPresenter == null) { | ||
_createLayoutPresenter(); |
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.
Instead of adding this conditional logic, can we create the layout presenter with a null default text style and then update it here without any conditional at all?
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.
I added this logic because there are some situations where we re-create the presenter inside didUpdateWidget
. For example, when the stylesheet changes.
It we always create the presenter with a null
default textstyle, then we need to replicate the code that sets the default text style everywhere we create the presenter.
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.
I didn't follow that explanation. Can you provide an example of what you mean?
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.
For example, if we want to create the presenter with a null
textStyle, we'd need to update the following places to also set the defaultTextStyle
after creating it.
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.
The first code snippet you included doesn't seem to reference the layout presenter at all. The second code snippet calls a method to create the layout presenter. Neither of these code snippets appear to show code that would need to be changed?
It would be great if you could show exactly what code snippet exists now, and exactly what code snippet it would need to become, so that we can avoid any confusion about what you're trying to say.
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.
Both snippets call _createLayoutPresenter
. Each one would need to change to the following:
_createLayoutPresenter();
_docStylesheetStyler.defaultTextStyle = DefaultTextStyle.of(context).style;
/// and builds a corresponding inline widget. | ||
/// | ||
/// If [defaultTextStyle] is non-`null`, the resulting [TextStyle]s | ||
/// will be merged with it. |
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.
Let's try to never say "merged" because it always begs the question as to which style wins out.
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.
Updated.
[SuperEditor][SuperReader][SuperTextField] Merge stylesheet styles with default text style. (Resolves #2727)
Currently,
SuperEditor
,SuperReader
andSuperTextField
don't honor the enclosingDefaultTextStyle
, which mean we cannot override the font family globally.This PR adds that ability.
EDIT: This was made as an opt-in feature for all widgets to avoid silently changing the styles for people that are relying on the currently no-inheriting behavior.