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

TextStyle extensions? #43

Open
esDotDev opened this issue Mar 30, 2020 · 12 comments
Open

TextStyle extensions? #43

esDotDev opened this issue Mar 30, 2020 · 12 comments

Comments

@esDotDev
Copy link

We recently added some extensions directly to TextStyle, and they are really handy:

extension TextStyleHelpers on TextStyle {
  TextStyle get bold => copyWith(fontWeight: FontWeight.bold);
  TextStyle get italic => copyWith(fontStyle: FontStyle.italic);
  TextStyle letterSpace(double value) => copyWith(letterSpacing: value);
}

Now, instead of
TextStyles.body.copyWith(fontWeight: FontWeight.bold, fontStyle: FontStyle.italic, letterSpacing: 1.6)
Can use:
TextStyles.body.bold.italic.letterSpacing(1.6)

I know these are technically not widgests, but I can't think of a better place to house these than this repo :)

@esDotDev
Copy link
Author

If you like the idea, I can do a PR for you sometime this week.

@esDotDev
Copy link
Author

We've created a lib here, if you'd like to include it in yours as a dependency that would be pretty cool :)
https://pub.dev/packages/textstyle_extensions

@ReinBentdal
Copy link
Owner

Cool package, however I am not really a big fan of all the shorthands. For example clr instead of color. I am also a little bit concerned about the performance of when copying the style for each textstyle parameter

@esDotDev
Copy link
Author

Ya I had to basically embrace the shorthand since there were so many conflicts with the underlying API.

@ReinBentdal
Copy link
Owner

I see. But I would rather prefer textColor instead of clr for example. Anyways I will keep this issue open for now and follow the development

@esDotDev
Copy link
Author

Ya that's fair, but we would end up with things like textBaseline and textDecoration and textWordSpacing. But ya I think you're right, that works a little better overall...

@esDotDev
Copy link
Author

I've updated the API with better naming, thanks for the feedback.

@ReinBentdal
Copy link
Owner

My main concern with this, just like with styled_widget, each time a method is called the object is copied, which might be a performance concern for bigger projects. To overcome this problem I think the way is to create a separate class which would make it look something line

CustomText
  .bold()
  .fontSize(24)
  .build()

where build returns a TextStyle object.
This might also be a solution in styled_widget

StyledText("Text")
  .bold()
  .fontSize()
  .build() // builds into Text object
  .padding(all: 10)
  // etc

@esDotDev
Copy link
Author

esDotDev commented Oct 25, 2020

Oh interesting, that would be better for performance, and also stop the fighting with the property names on TextStyle. I agree, much better :)

The downside to this approach is it doesn't lend itself well to pre-declaring. Typically we would have a bunch of shared TextStyles:

class TextStyles {
TextStyles baseFont = TextStyle(...);
TextStyles h1 = baseFont.fontSize(24).bold();
}

But that's probably ok, as long as we can use it inline in the Widget's themselves, I think that's the primary use case.

@ReinBentdal
Copy link
Owner

Actually it would then be better in the example I gave to use double dot before the method (https://dart-lang.github.io/linter/lints/avoid_returning_this.html)

CustomText // example name
  ..bold()
  ..fontSize(24)
  .bold()

@sooxt98
Copy link

sooxt98 commented Mar 18, 2021

Just created this, hope it helps https://github.com/sooxt98/textless 😉

@ReinBentdal
Copy link
Owner

@sooxt98 Maybe we should be able to merge the package into this? You are using extra as a map in the package to be able to modify parameters in a stateless widget, right? I think there might be a performance hit since each method returns a new ThemedText.
For it to be merged I think this should be changed:

  • Changing the map to a class with mutable member variables. This just makes it a little easier to make the code rigorous in my opinion and is more in line with the Dart/Flutter code style.
  • Change it so that each method doesn't return a completely new widget, but instead modifies the class with mutable member variables. The solution would then be to have a method which converts the object to a widget, as previously described in this issue.

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

No branches or pull requests

3 participants