chec / ui-library

Chec UI library assets and components
https://chec-ui.netlify.app/
BSD 3-Clause "New" or "Revised" License
21 stars 6 forks source link

Change mIcon's plusname property to 'plus' #94

Open john-raymon opened 4 years ago

john-raymon commented 4 years ago

Within dashboard-beta this allows me to simply pass the plan_name property to <ChecMarketingIcon> without having to worry about appending the word plan when plan_name is plus.

Want to be able to do:

const prop_name = 'plus'; // works for standard, pro, rise, growth. 
<ChecMarketingIcon
  :icon="plan_name"
  class="w-full h-auto"
/>

But can't because of plusplan property name in

https://github.com/chec/ui-library/blob/master/src/lib/icons.js#L64

robbieaverill commented 4 years ago

I'm not sure I agree with this. I can see why you're suggesting it. We should be careful not to couple things too much - example: if you pass the API's value straight through to the ui-library then you might end up with a broken icon if we suddenly add a new plan. Instead you should code in the expected values and how they map to the ui-library.

The icons in that list include icons that aren't for plans, and there are some other "plus" icons there, which is probably why it's been named "plus plan" to help differentiate it.

john-raymon commented 4 years ago

@robbieaverill But the pattern is already being followed by the mIcons. Changing plusplans to plus doesn't make much of a difference.

By changing plusplan to plus, in my opinion it's only making the mIcons property names consistent if anything, since they're already following the same naming as the plans.

Should we map the plan names with the icon names on the client-side in the vuex action that fetches plans then? or within the plan-table component that will render plans? Rather than changing this property name.

from this

export const mIcons = {
  standard: IcStandard,
  plusplan: IcPlusPlan,
  pro: IcPro,
  rise: IcRise,
  growth: IcGrowth,
  enterprise: IcEnterprise,
  standardcircle: IcLrgStandard,
  pluscircle: IcLrgPlus,
  procircle: IcLrgPro,
  risecircle: IcLrgRise,
  growthcircle: IcLrgGrowth,
  enterprisecircle: IcLrgEnterprise,
};

to this

export const mIcons = {
  standard: IcStandard,

  plus: IcPlusPlan,

  pro: IcPro,
  rise: IcRise,
  growth: IcGrowth,
  enterprise: IcEnterprise,
  standardcircle: IcLrgStandard,
  pluscircle: IcLrgPlus,
  procircle: IcLrgPro,
  risecircle: IcLrgRise,
  growthcircle: IcLrgGrowth,
  enterprisecircle: IcLrgEnterprise,
};
robbieaverill commented 4 years ago

My opinion is that the plan icons should be prefixed or suffixed with plan, maybe that's the fix here? I still don't think you should blindly pass the API plan name through to the icon component though

ScopeyNZ commented 4 years ago

I think there's definitely something to do here. All the "plan" icons don't have the word "plan" except plusplan. We should make them all have plan, or none of them imo.