Add base class for all styles

[CodeDoctorDE]

Closes #17 . I can also work on #16 if you want

[CodeDoctorDE]

@rurickdev What do you think?

[rurickdev]

This looks really cool. Is approved from my side, but I think only @rektdeckard can merge the branch

Also, I'm a little busy this morning, if you can work on #16 too that would be great!

[CodeDoctorDE]

Hmm, I found out, const isn't very nice to work with these abstract classes, I will look at it to fix both

[CodeDoctorDE]

Hmm, I think we cannot remain the PhosphorIcons.regular structure with static classes, unless we need to do something like

static const _address = ...;
final address = _address;

but I don't think this would be nice

But I would do something like

PhosphorIconsRegular.plus

or

PhosphorIcons.plus(PhosphorIconsStyle.regular)

[CodeDoctorDE]

Update: I implemented the second option in this commit. Share your opinion!

But this would be the second breaking change

[CodeDoctorDE]

Hi can somebody test if the example app builds on android? I have currently no android sdk installed

[rurickdev]

@CodeDoctorDE I don't know if that's the best approach, afaikn the idea is to have the same style of API for all the Phospho libraries and this will definitely break that idea. @rektdeckard can you confirm that?

I would prefer this option

PhosphorIconsRegular.plus

rather than this

PhosphorIcons.plus(PhosphorIconsStyle.regular)

the first options is what the community package of font awesome does https://github.com/fluttercommunity/font_awesome_flutter/blob/master/lib/font_awesome_flutter.dart

and for example, both MDI community icons package (and all the icon packas i know about) does the previous sintax with the style as subfix of the icon https://github.com/pepsighan/mdi-dart/blob/master/mdi/lib/mdi.dart https://github.com/ziofat/material_design_icons_flutter/blob/master/lib/material_design_icons_flutter.dart

the second syntax would include too much boiler plate for the users.

[CodeDoctorDE]

I had added both systems, if you want to use it. I added this complicated system too, to allow something like: PhosphorIcons.plus(selected ? PhosphorIconStyle.filled : PhosphorIconStyle.regular)

[rurickdev]

What improvement bring doing this

PhosphorIcons.plus(selected ? PhosphorIconStyle.filled : PhosphorIconStyle.regular)

over this

selected ? PhosphorIconsFilled.plus : PhospohorIconsRegular.plus

[CodeDoctorDE]

I build my app very modular. For example I have this abstract class

abstract class Handler {
PhosphorIconData getIcon({bool filled = false});
}

With this implementation I can further improve the ui and more, I can use this new Style enum in the parameter to easily choose the icon for the current ui. It would help me a lot, if there is a connection between these styles with the same icon.

An example with the new base PhosphorIcons class

class PenHandler extends Handler {
PhosphorIconData getIcon = PhosphorIcons.pencil;
}

But you don't need to use it if you don't want it, I added export for all style classes to use for example PhospohorIconsRegular.plus

[rektdeckard]

As a non flutter dev I don't feel qualified to have a real opinion here, so I'm happy with whatever you all agree to as long as it follows the principle of least astonishment for users :)

[rektdeckard]

@rurickdev I have given you write access on this repo, BTW

[rurickdev]

That sound really handy for some cases. I would just ask if you are sure that all that static getters are tree shaked (I believe it does), if not then the package could be adding a lot of code that maybe many apps don't need, also, this feature will not be in other phospho libraries, something that will (maybe) force other packages to implement, but again, thats part of some restrictions that @rektdeckard knows better than me.

at the end, I'm not against it, looks like a good feature, just wonder why other icons packages don't have something similar, makes me think...

also, in your example, I still don't see the advantages

abstract class Handler {
  PhosphorIconData getIcon({bool filled = false});
}

class PenHandler extends Handler {
  PhosphorIconData getIcon = PhosphorIcons.pencil;
}

class NoteHandler extends Handler {
  PhosphorIconData getIcon({bool filled = false}) => filled ? PhosphorIconsFilled.note : PhosphorIconsRegular.note;
}

buuuut I could see them if there is anyone that has created some kind of design system / themeData / template where they just needs to pass the style and all the icons in the app changes to that.

/// example using riverpod

/// pod that controls the style state
final iconsStylePod = StateProvider((_)=>PhosphorIconStyle.regular));

/// some consumer widget
Icon(
  PhosphorIcons.pencil(
    style: ref.watch(iconsStylePod),
  )
),

/// some user settings options callback
onIconStyleChange: (PhosphorIconStyle style){
  ref.read(iconsStylePod.notifier).state = style;
}

Maybe the only thing I'll add is a default option in the getters, so the users can use this option without having to pass a style always. Also, please add docs in the class and methos that clarify the difference on why use PhosphorIcons.pencil(style) or PhosphorIconsRegular.pencil

and update the migration guide in the readme to notify about this feature and how the user can use it.

and @rektdeckard can you confirm this new feature dont break the idea of the same API for all the packages, or if there is no problem to have it just here?

for my side, I'll be happy to approve it when the previos comments are solved

[CodeDoctorDE]

Sure, I can add this default option. Should I just use regular as default or what do you mean? PhosphorIconData pencil([PhosphorIconStyle style = PhosphorIconStyle.regular]) I will add these docs tomorrow

[rurickdev]

yeah, I think regular is the way to go

[CodeDoctorDE]

Or you can also make a special PhosphorIcon widget that has a function as icon attribute: final PhosphorIconData Function(PhosphorIconStyle) icon; property and the style will be get from the widget tree

[rektdeckard]

and @rektdeckard can you confirm this new feature dont break the idea of the same API for all the packages, or if there is no problem to have it just here?

I think we do what we can. It's fine as long as we have:

1. Smart defaults (regular weight is the default)
2. Support for all features available to other libs (we now have duotone, so satisfied)
3. Idiomatic (does not surprise users in how it is consumed)
4. Performance/size do not suffer terribly from 1-3

[rurickdev]

I think we do what we can. It's fine as long as we have:

I believe this proposed changes satisfies all points

---

Or you can also make a special PhosphorIcon widget that has a function as icon attribute: final PhosphorIconData Function(PhosphorIconStyle) icon; property and the style will be get from the widget tree

that sounds like a good idea, but I don't know if it should be in this package or maybe other one that depends on this to keep the single responsibility (and like other packages that have extensions/plugins)

for the freature I think something like:

PhosphorStyleProvider( //don't like this name but couldn't think in another one
  style: PhosphorStyle.regular,
  child: MaterialApp(...),
);

then do something like final style = PhosphorStyle.of(context).style to read the style

the only weird thing I could think of the function approach is how to direct the users to do

PhosphorIcon( // there is already a widget in the package, is the only way to support duotone
  icon: PhosporIcons.pencil,
),

instead of

PhosphorIcon(
  icon:(style) => PhosporIcons.pencil(style),
),

or worst

PhosphorIcon(
  icon:(style){
    // more code and logic 😨 
    PhosporIcons.pencil(style: style);
  }
),

either way I think we are diverging the discussion, I'l add the comments to the code so we can keep track of them, but after resolved we can merge the PR