digidem / comapeo-desktop

GNU General Public License v3.0
2 stars 0 forks source link

chore: Adds a text component #28

Closed cimigree closed 1 month ago

cimigree commented 2 months ago

closes #17

Description

Small fixes

Screenshot 2024-07-04 at 5 22 02 PM

gmaclennan commented 1 month ago

Optional of course, but there is the DRY option, although sometimes these can be too "clever" vs. just typing things out.

const mapKindToMUIVariant = {
  body: "body1",
  title: "h1",
  subtitle: "subtitle1",
} as const

export function Text({
   kind 
   bold,
   italic,
   style,
   underlined,
   ...passThroughProps
 }: TextProps) {
   const muiVariant = mapKindToMUIVariant[kind]
   if (!muiVariant) throw ...
   return (
     <Typography
       variant={muiVariant}
       style={{
         ...style,
         fontWeight: bold ? 'bold' : undefined,
         fontStyle: italic ? 'italic' : undefined,
         textDecoration: underlined ? 'underline' : undefined,
       }}
       {...passThroughProps}
     />
   )
achou11 commented 1 month ago

Optional of course, but there is the DRY option, although sometimes these can be too "clever" vs. just typing things out.

yeah upon further reflection, this may be fine for our needs. i had suggested the more repetitive approach because it had advantages that - in retrospect - may not be super relevant:

gmaclennan commented 1 month ago
  • can import specific text kind components directly, in the case where you don't need the "dynamism" of a singular text component (e.g. unlikely you'd want to switch some text between title and body styling)

I think this is just an API preference - <Text variant="body"/> vs <BodyText /> - I see different component libraries choose one of the other. I don't mind. You can still do this DRY with const BodyText = (props) => <Text kind="body" {...props} /> but repitition also fine!

  • easier to implement different behavior if needed because of the separation in implementation. think it would be pretty rare that this would happen (why would i want different component behavior between e.g. title and body text?? 😄 )

I don't think we should try to think through too many "possible scenarios" until we actually need them. Let's try and keep the surface area of the components we build to the minimum and add as needed, so if we do change MUi there is not much that we are depending on in the actual code.

cimigree commented 1 month ago

OK I think this is what you want?