digidem / comapeo-mobile

The next version of Mapeo mobile
GNU General Public License v3.0
5 stars 1 forks source link

chore: updated buttons and some headers and labels to use rubik #818

Closed ErikSin closed 3 weeks ago

ErikSin commented 3 weeks ago

closes #769

Added rubik font via expo. Updated the text component to take a variant prop. Setting variant=primary will use font rubik Updated the button component to use variant "primary" Updated selected header and labels to use variant "primary" Update the nav header to use Rubik

Updated these headers and labels: https://www.figma.com/design/iUeC0Qzhb4H0unuPfxQAsM/(Mobile)-CoMapeo?node-id=3468-2389&node-type=frame&t=SFrkpMNpu1M2cDRi-0

To Do:

ErikSin commented 3 weeks ago

Although most of the screens shown in the Figma are addressed by this PR, I am pretty confused about how this PR resolves all of the issues in #769 . I have included some screen shots to show what seems on first check to not be fixed. One thing for sure is the truncation is not addressed at all, is it? The issues says "Make sure all titles and labels are using Rubrik" It also says: "Correct font sizes throughout, ensure type ramp is applied" and "Apply truncation rules to areas that risk overrunning"

Sorry, I shouldn't have tagged the original issue. This fixes only certain screens that were addressed by Sabella in call. WE identified that doing all the screens at once would be too much work so she wanted me to to these screens first.

Also, it seems like there are a number of ways to do the styling in a way that is more easy to maintain in the future and for future developers. One way would be to use a centralized theme or style sheet (a styles.ts file where you could have a heading, label, body, etc and call them rubikheader or whatever) Or another idea would be to make specialized text components, eg. heading, label, bodytext etc In addition to being confused about and not loving the changes to Text.tsx I am just concerned about future implementation and how much of it there will be and how the issue does not seem resolved.

So I am open to other ways of doing this.

For the first alternative of having a theme, are you suggesting we have a styles.ts that looks like:

const styles = Stylesheet.create({
     rubikHeader:{fontFamily: rubik}
})

export rubikHeader = styles.rubikHeader

and then in the component we would do

<Text style={rubikHeader}>...someText</Text>

If this is what you are suggesting, could you just elaborate on why you think that is a better alternative?

For the second alternative, im also curious what the advantages of having a < Header > vs a < Text variant='header' > is? To me this seems like the same approach, with a slightly different implementation.

Im open to either of these, just curious as to why you think they are better alternatives :)

cimigree commented 3 weeks ago

Yes, your example is what I was suggesting. If we have a styles.ts file with predefined styles, it might make it easier to apply the styles across the app. Like this maybe?

export const typography = StyleSheet.create({
  header: {
    fontFamily: 'Rubik_500Medium',
    fontSize: 24,
    // other header styles
  },
  label: {
    fontFamily: 'Rubik_400Regular',
    fontSize: 16,
    // other label styles
  },
  bodyText: {
    fontFamily: 'System',
    fontSize: 14,
    // other body text styles
  },
});

Then use it like this:

import { typography } from './styles';

<Text style={typography.header}>Some Header Text</Text>

Are we using variant "header"?

That could be ok. But, if it's its own component, then there can be additional logic in there if needed...

Something like:

export const Header = (props: TextProps) => (
  <Text style={typography.header} {...props} />
);

export const Label = (props: TextProps) => (
  <Text style={typography.label} {...props} />
);

And then use it like so:

<Header>Welcome</Header>
<Label>Username</Label>

Doing this would make the code more semantic and then developers don't have to remember about additional logic (the word primary is that your choice?). Also, everything would be centralized and if we change it at the top level it would be applied throughout. Having to pass variant="primary" or the exact fontweight seems much more fallible and to me -- a temporary solution only. I thought we try to develop with the future in mind even if you can't do everything requested in the ticket. Wouldn't it have been nice if we had these set up already and then if you pulled Rubik in it would have been applied in all of the screens and components? You are the expert, obviously, but even if you don't want to refactor, could you fix the one screen that does not have the bold text as it is in Figma for this?

ErikSin commented 3 weeks ago

Yes, your example is what I was suggesting. If we have a styles.ts file with predefined styles, it might make it easier to apply the styles across the app. Like this maybe? export const typography = StyleSheet.create({ header: { fontFamily: 'Rubik_500Medium', fontSize: 24, // other header styles }, label: { fontFamily: 'Rubik_400Regular', fontSize: 16, // other label styles }, bodyText: { fontFamily: 'System', fontSize: 14, // other body text styles }, });

Then use it like this: import { typography } from './styles';

Some Header Text

Are we using variant "header"?

That could be ok. But, if it's its own component, then there can be additional logic in there if needed...

Something like: export const Header = (props: TextProps) => ( <Text style={typography.header} {...props} /> );

export const Label = (props: TextProps) => ( <Text style={typography.label} {...props} /> );

And then use it like so: Welcome Username

Doing this would make the code more semantic and then developers don't have to remember about additional logic (the word primary is that your choice?).

I agree that having a text component, which is declarative, would be ideal (whether its < Header /> or < Text variant="header" />). The only issue is the delineation between what is a header vs label vs body is not as simple. Something can be a label, but can be bolded differently from another label, for example. Or another common example is something can be a header, but sized differently from another header (page header vs section header).

We could break up all of these categories into different component, ie < Label1 />, <Label 2 />, < PageHeader / >, < SectionHeader /> < BodyLarge /> < BodyRegular />....etc

But i am hesitant to do that because it would require a lot of mental overhead of knowing which styling belongs to which component. According to the design system there are 10 different variants AND none of those include how its bolded.

So my mentality (which is not say I am right) is that the developer cares about the font, the weight, and the size. There is already properties in style that deal with weight and size, so I wanted to keep that the same (hence my getFontFamily() function => allowing the developer to just use fontWeight normally). And then all the developer need to do is determine if the font is rubik (aka "primary") or the default font.

That being said, I see that manually setting the font weight and font inside each component is not the best for scalability and maintenance. Im open to alternatives, but I dont think having 10 different components is helpful, especially if those components really only deal with the size and font, but not font weight. Open to suggestions if you see a middle ground

cimigree commented 3 weeks ago

Sure what you say makes sense. Is the term "primary" a choice you made? I am not crazy about it as providing clear meaning of what it is/ does. Can that at least be something else? Also, still missing that bolded text see screen shot above.

ErikSin commented 3 weeks ago

So I spoke to Sabella Today. I misunderstood the requirements. All headers, labels, and buttons, use the same font (rubik) and the same font weight (500). All body text, uses the system font and the regular font weight. The only difference is between all the variant are the font sizes.

So based on that new information I did what you suggested and created semantic components. I opted to use< Header varinant="{someVariant}" /> as opposed to < Header1 />, <Header2 /> etc. The reason I did that was to take advantage of JS doc. So when a developer uses the HeaderText or BodyText component, they can put there mouse over the component and VS code will tell them which variant is which size. This will avoid having to open the file each time to determine what variant is which size.

I also added the @deperecated tag to our Text component. I think it will be too much in one PR to replace all the Text components. So we should do it for every file we touch. The deprecated tag will notify us that it should be replaced when we open a file.