added font-variant to global font type #6327
Conversation
There was a problem hiding this comment.
Thanks for working on this!
I think the following needs added to the src/js/utils/styles.js file in baseStyle so that we make sure the font variant we are passing through the theme actually gets applied:
font-variant: ${(props) => props.theme.global.font.variant};
After that try running yarn test-update to update the testing snapshots
|
hi @jcfilben added font-variant,updated the tests, but still tests for some components are failing, do let me know what can be done. And thanks for being patient and taking out the time to look at this PR! Appreciate it |
Hmm I'm not sure why the tests are acting strange. I'm going to try and just update the branch on my end and then push that up to this branch and see if that gets the tests to re-run |
|
@jcfilben checks are passing now ! Thank you ! Do let me know if any other changes and/or tests needs to be updated before merging. |
src/js/utils/styles.js
Outdated
| @@ -9,6 +9,7 @@ export const baseStyle = css` | |||
| font-size: ${(props) => props.theme.global.font.size}; | |||
| line-height: ${(props) => props.theme.global.font.height}; | |||
| font-weight: ${(props) => props.theme.global.font.weight}; | |||
| font-variant: ${(props) => props.theme.global.font.variant}; | |||
There was a problem hiding this comment.
What happens if there is no props.theme.global.font.variant, do we still include this in the CSS? If so, I'm wondering if we should make these more conditional.
There was a problem hiding this comment.
@jcfilben could you give some inputs?
There was a problem hiding this comment.
I think we could change these to be mode conditional by doing something like this, where we first check if the prop is defined in the theme before adding the css.
${(props) => props.theme.global.font.variant && `
font-variant: ${props.theme.global.font.variant};
`}
There was a problem hiding this comment.
@jcfilben done, fair warning tests are not passing, updated them using yarn test -u, still failing, do let me know what I have to correct and I'll correct it
There was a problem hiding this comment.
Strange I'm not sure what is going on in your local environment that is causing the tests to behave oddly. I will go ahead and do the same thing we did last time and update the branch from my end to get the tests to re-run.
If you create any future pull requests it might be worth starting with a fresh fork of grommet to see if that resolves any issues with the tests, but no need to do that for this pull request.
There was a problem hiding this comment.
@jcfilben noted ! any future PRs will be with a fresh fork! Thanks for taking out the time to update the branch. Let me know if anything else needs to be done for this PR
|
@joybh98 thanks for contributing to Grommet! |
What does this PR do?
Adds the
font-variantoption to the global font typeWhere should the reviewer start?
src/js/themes/base.d.tsWhat testing has been done on this PR?
yarn test, tests failingHow should this be manually tested?
Do Jest tests follow these best practices?
screenis used for querying.userEventis used in place offireEvent.asFragment()is used for snapshot testing.Any background context you want to provide?
NA
What are the relevant issues?
#6289
Screenshots (if appropriate)
Do the grommet docs need to be updated?
Should this PR be mentioned in the release notes?
Is this change backwards compatible or is it a breaking change?