feature: Added font-weight support #165
Conversation
| @@ -52,6 +55,7 @@ class RendererModel | |||
| /** @var array<string, string> $DEFAULTS */ | |||
| private $DEFAULTS = [ | |||
| "font" => "monospace", | |||
| "weight" => "400", | |||
There was a problem hiding this comment.
In demo/js/script.js there is also a place to add defaults for the demo site that makes the URL shorter when the default value is used.
If you could add weight: "400", to the defaults object in that file as well that would be great. You can edit that on your forked branch.
(Btw for the future, it's typically better to create a new branch in your fork before working on a feature, rather than pushing to main since it allows you to have main kept up to date with the original repo and work on multiple features on separate branches. It's totally fine in this case, just a tip for going forward 😄).
There was a problem hiding this comment.
Will do!
Thank you very much for the detailed feedback. I actually did work on a separate branch "feat-font-weight" that had only 2 commits, but I merged it to my "main" when I finished and used that for the PR. So the ideal would be to PR my "feat-font-weight" directly to your "main"?
There was a problem hiding this comment.
Yeah, best is to create a branch for the PR and create the PR from that feature branch to the upstream repo's main. Then updating that feature branch will also update the PR.
There was a problem hiding this comment.
I just updated the demo default on a new branch and merged into my main, not sure if I did it the proper way 😬
There was a problem hiding this comment.
As long as it all ends up on the right branch eventually, it's fine.
In this case the source branch of the PR is main, so all new commits to the PR would go there.
If you created the PR from a separate branch, then that separate branch would be where the new commits go.
Summary
Feature
Added option to specify font-weight, as available on Google Fonts.
Type of change
How Has This Been Tested?
It has been tested locally, manually and by running
composer testI believe it's not necessary to create additional tests for this feature
composer test