Calendar - add calendar[size].title prop to theme
#6639
Conversation
|
I'm wondering if |
Good note, I'll see if I can find a different name for this |
renamed to titleSize |
| })} | ||
| </Heading> | ||
| {theme.calendar.heading && theme.calendar[size].titleSize ? ( | ||
| <Text weight="bold" size={theme.calendar[size].titleSize}> |
There was a problem hiding this comment.
Enforcing bold here might not be the best idea because in v5 of HPE theme, we are leaning away from use of bold in typography. Wondering if we should have theme.calendar[size].title where any text props are supported?
In base.js, the default weight might be bold but then in HPE theme we can override with 500 or whatever is appropriate?
There was a problem hiding this comment.
Wondering if we should have theme.calendar[size].title where any text props are supported?
Done in latest commit
| @@ -589,23 +590,36 @@ const Calendar = forwardRef( | |||
| return ( | |||
| <Box direction="row" justify="between" align="center"> | |||
There was a problem hiding this comment.
Any reason this isn't a <Header />?
There was a problem hiding this comment.
Changed Box to Header in latest commit
| year: 'numeric', | ||
| })} | ||
| </Heading> | ||
| {theme.calendar.heading && theme.calendar[size].titleSize ? ( |
There was a problem hiding this comment.
Why do we need the check for theme.calendar.heading?
There was a problem hiding this comment.
Removed this extra check in latest commit
| </Heading> | ||
| {theme.calendar.heading && theme.calendar[size].titleSize ? ( | ||
| <Text weight="bold" size={theme.calendar[size].titleSize}> | ||
| {reference.toLocaleDateString(locale, { |
There was a problem hiding this comment.
Since we're duplicating this code, can we put it in a const variable first?
There was a problem hiding this comment.
Yes, fixed in latest commit
| })} | ||
| </Heading> | ||
| {theme.calendar.heading && theme.calendar[size].titleSize ? ( | ||
| <Text weight="bold" size={theme.calendar[size].titleSize}> |
calendar[size].headingSize prop to themecalendar[size].title prop to theme
What does this PR do?
Adds a new prop
calendar[size].headingSizeto the theme. Specifying this prop will override thecalendar.heading.levelprop and change the calendar to use<Text>instead of<Heading>to display the date and year. This prop also gives the flexibility to specify different font sizes for each calendar size (small, medium, and large).In addition to this PR I will file a PR on grommet-theme-hpe to opt into this behavior for the hpe theme.
Where should the reviewer start?
What testing has been done on this PR?
Added a jest test and tested by adding a custom theme to calendar stories
How 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?
What are the relevant issues?
Related to #6637
Screenshots (if appropriate)
Do the grommet docs need to be updated?
Yes, we need to document the new theme prop as well as mention that using
calendar[size].headingSizeis preferred overcalendar.heading.levelShould this PR be mentioned in the release notes?
Yes
Is this change backwards compatible or is it a breaking change?
Backwards compatible