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

custom theme style improvements and fixes #113

Closed
wants to merge 57 commits into from

Conversation

gdfreitas
Copy link
Contributor

@gdfreitas gdfreitas commented Oct 18, 2024

Changes

Source Issue #111

  1. Fixes font-style property being used for setting font-weight values
    1.1 Now it supports both font-style and font-weight properties, defaults were kept.

  2. Adds support for customizing a few properties from existing style objects, as well as new ones introduced in this merge request.

  3. CustomThemeStylesChart story was added just as a reference, it can be removed before incorporating this pull request, if you feel so. I thought it would be nice to have a dedicated story to show how customizable the chart is apart of existing themes.

@eatyourpeas

@gdfreitas gdfreitas marked this pull request as ready for review October 18, 2024 14:27
@eatyourpeas
Copy link
Member

Hi @gdfreitas I am really sorry polluted this PR a bit - meant to bring it up to date with live before reviewing (I have updated a few things recently) and ended up in merge-conflict purgatory. This all looks great and I am generally happy with it. The story is good to have - if you are happy to add to the RCPCHChart.mdx with some documentation about what you have done and so on would be great. Really useful.
A couple of things - the fonts currently in the component are only montserrat regular, bold and italic. Their weights I think therefore are 400 and 700 and these are the only ones included in the fonts folder. Probably for this reason I am not seeing an impact if I change the prop. I did this partly because we did not need the range of weights. Do you need specific font weights or fonts? If so do you want to add these? They would need converting to b64 and then importing in RCPCHChart.tsx in the GlobalStyle styled-div.
I think best is if you are happy to look at these small items and resubmit a fresh PR and I will be less of a klutz with that one and merge into live, rather than the other way round that turned out more complicated than I expected.

@gdfreitas
Copy link
Contributor Author

Hi @eatyourpeas as we discussed, I'm closing this in favor of #117, which is still a work in progress for now. Thank you.

@gdfreitas gdfreitas closed this Oct 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants