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

add new custom theme properties for styling #117

Merged
merged 7 commits into from
Oct 24, 2024

Conversation

gdfreitas
Copy link
Contributor

@gdfreitas gdfreitas commented Oct 22, 2024

Feature

This merge request turns a few styling properties public API, making some parts more flexible for consumers that are using custom themes

Changes

  1. Adds weight for base TextStyles interface, which was previously documented under style causing some applications to not effectively work, because in CSS they're a two different properties. Also, now there's a benefit that we're not making restrictions, bold/bolder/300/400, any supported CSS values can be used. Defaults were preserved to maintain compatibility with current implementations
  2. Makes text elements for thresholds like nondisjunction and delayed puberty automatically adjust its distance from the line on Y axis, based on font-size.
  3. Allows changing color of axis thresholds lines through customThemeStyles.axisStyle.axisThresholdLineStyle
  4. Allows editing chart reference text style through customThemeStyles.referenceStyle
  5. Allows customizing centile text style through customThemeStyles.centileStyle.centileTextStyle
  6. Allows customizing toggle tooltips colors and texts
  7. Adds a CustomThemeStylesChart on RCPCH story in Storybook to be used as a more complete reference of pieces that can be customized

Thoughts on Fonts for Future

I would like to share some thoughts on how the fonts are being handled by the library. Related comment #113 (comment)

Currently the library embeds a few variants of Montserrat font using base64 encoding to include in the built assets, for later injecting a global style to define the font faces in CSS.

Pros

  • Makes the library have a good accessibility font by providing a well tested Typography
  • Consumers of themes wouldn't have to worry about installing/managing fonts in their application

Cons

  • Makes the library way bigger than it needs to be
  • The embedded fonts won't be used by those who are using other font families.
  • Doesn't scale well if user needs a different set of font, it would require maintainers to keep adding embedded fonts to the library. (related to MR feedback)

My suggestion is to decouple fonts managing from the library, this would require:

  • Not embedding fonts in the library anymore
  • If you feel necessary, document recommended typography (e.g: Montserrat) and how consumers could install on their projects, for example using CDNs script links, using Vite/Next.js specifics, etc.
    • I mean't "if you necessary" because anyone using React could quickly research about installing fonts on the web and dive deeper into their specific needs, e.g: Next.js/Vite/CDN, etc.

@eatyourpeas Please, let me know your thoughts on this

@gdfreitas gdfreitas force-pushed the custom-theme branch 2 times, most recently from bfb4b8f to ff3fdcc Compare October 22, 2024 16:50
@gdfreitas gdfreitas marked this pull request as ready for review October 22, 2024 19:43
@gdfreitas
Copy link
Contributor Author

gdfreitas commented Oct 22, 2024

Hi @eatyourpeas I would also encourage you guys to file a separate merge request after this one being merged to run eslint/prettier to format the whole project, or even better, including this task on a pipeline or precommit hooks. I had to disable all vscode formatting to be able to contribute with this MR without bloating it with this kind of changes 😬

@gdfreitas
Copy link
Contributor Author

I pushed one more customization for tooltips.

By the way, I'm trying to keep up with current source code naming and patterns. E.g: I'm using colour instead of color for public APIs because that's something I noticed in the code base.

Let me know if I need to adjust something.

@eatyourpeas eatyourpeas added the feature request New feature or request label Oct 23, 2024
@eatyourpeas
Copy link
Member

Thanks @gdfreitas for your observations here - all very helpful.
I am interested to know @mbarton take here.
For the fonts, our users pretty much all use the pre-rolled themes but then they are catering to a UK market where people are familiar with the RCPCH brand and are not looking to customize: you are the first users we have had that have been keen to match the chart to their own brand. It was with this in mind that we bundled the fonts into the library which I know is not standard practice. I noticed a while back that a user who was in the late stages of implementation had times new roman in the tooltips which didn't look right so we took this path for that reason. For sure it has an impact on bundle size, but actually the growth references themselves also take up quite a lot of space so at this point unless we start dynamically pulling those in and caching (there is an issue open for this) bundle size has not been a big priority - particularly as we are targetting organisations that will be loading on desktop over wired networks in the main.
Re: prettier/eslint - yes I agree. We did have this in in the past. Not totally sure how it got removed.

@mbarton - what do you think? if we took the fonts out we would have to ask users to import into their different environments.

Otherwise, everything else sounds very sensible. I will have a look in a bit more detail.

@gdfreitas
Copy link
Contributor Author

Hi @eatyourpeas I understand, we can move the fonts discussion to a separate to not block this PR to be reviewed and merged.

It's not an issue for now.

@eatyourpeas eatyourpeas self-requested a review October 23, 2024 17:44
Copy link
Member

@eatyourpeas eatyourpeas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @gdfreitas - this is very good. Looking at it now I can see that some of the naming could be better organised. I agree opting for color rather than colour makes more sense. The axisStyles and the nondisjunction line styles and delayed puberty line styles have become mixed.
I think if you are happy it meets your use case for the moment let's merge in but make a mental note that could use a refactor. Like many projects, it has evolved over time and needs a tidy.

@eatyourpeas
Copy link
Member

@gdfreitas just seeing tests failing and a bunch of errors in the browser console. Will need to unpick this before merging - hope that is ok.

@gdfreitas
Copy link
Contributor Author

Hi @eatyourpeas I just noticed too, seems I'll need to fill up some test parameters for the new styles because we're directly mocking for tests. I'll do that right now.

@gdfreitas
Copy link
Contributor Author

gdfreitas commented Oct 24, 2024

@eatyourpeas It should be fine now, there is still some warnings that are on the live branch too, shouldn't be related to this PR.

Commit 64cc9a8

@eatyourpeas eatyourpeas merged commit af45a65 into rcpch:live Oct 24, 2024
@mbarton
Copy link
Member

mbarton commented Oct 25, 2024

I just want to say thank you @gdfreitas for all your contributions here - they are massively appreciated!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants