Skip to content

add level prop for Calendar #7456

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Dec 10, 2024
Merged

add level prop for Calendar #7456

merged 4 commits into from
Dec 10, 2024

Conversation

britt6612
Copy link
Collaborator

What does this PR do?

This PR add level for the prop in Calendar so the user can be able to change the level of the Heading in Calendar without using the theme

Where should the reviewer start?

calendar.js

What testing has been done on this PR?

locally and added a test and story

How should this be manually tested?

storybook

Do Jest tests follow these best practices?

  • screen is used for querying.
  • The correct query is used. (Refer to this list of queries)
  • asFragment() is used for snapshot testing.

Any background context you want to provide?

A user should be able to pass level to the Calendar component to control the heading level used.

What are the relevant issues?

closes #7450

Screenshots (if appropriate)

Do the grommet docs need to be updated?

yes level needs to be added

Should this PR be mentioned in the release notes?

yes

Is this change backwards compatible or is it a breaking change?

backwards compatible

@britt6612 britt6612 requested a review from jcfilben December 5, 2024 20:17
jcfilben
jcfilben reviewed Dec 5, 2024
View reviewed changes
src/js/components/Calendar/Calendar.js Outdated
Comment on lines 641 to 642
// theme.calendar.heading.level should be removed in v3 of grommet
// theme.calendar[size].title should be used instead
Copy link
Collaborator

@jcfilben jcfilben Dec 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Could we move this comment up to line 628 so it is closer to where we are using theme.calendar.heading.level

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

src/js/components/Calendar/stories/HeaderLevel.js Outdated
};

export default {
title: 'Visualizations/Calendar/HeaderLevel',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we change the story name to 'Heading level' with a space and lowercase 'l' to match the other stories?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

@britt6612 britt6612 requested a review from jcfilben December 5, 2024 20:46
jcfilben
jcfilben previously approved these changes Dec 5, 2024
View reviewed changes
Copy link
Collaborator

@jcfilben jcfilben left a comment

Choose a reason for hiding this comment

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

Looks good!

@jcfilben jcfilben dismissed their stale review December 5, 2024 21:10

Missing one thing on the story

@jcfilben
Copy link
Collaborator

jcfilben commented Dec 5, 2024

Actually one more thing, looks like the story name is being weird:

Screenshot 2024-12-05 at 2 09 58 PM

Try using .storyName to set the story name

jcfilben
jcfilben approved these changes Dec 5, 2024
View reviewed changes
Copy link
Collaborator

@jcfilben jcfilben left a comment

Choose a reason for hiding this comment

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

Looks good!

@britt6612 britt6612 requested a review from taysea December 6, 2024 02:20
@taysea
Copy link
Collaborator

taysea commented Dec 6, 2024

I thought we had added calendar[size].title to allow for more control of the sizing and to render not as a heading because we'd determined heading wasn't necessarily the appropriate text element for Calendar? #6639

If so, should we direct people towards using that?

@britt6612
Copy link
Collaborator Author

@jcfilben @taysea
For grommet theme for backwards compatibility we can’t switch to use calendar.large.title in the theme so we need another way to control the heading size

@jcfilben
Copy link
Collaborator

jcfilben commented Dec 9, 2024

I thought we had added calendar[size].title to allow for more control of the sizing and to render not as a heading because we'd determined heading wasn't necessarily the appropriate text element for Calendar? #6639

If so, should we direct people towards using that?

We did add the calendar[size].title prop to the theme and are using this in the HPE theme, the issue is for the Grommet theme to maintain backwards compatibility we can't switch to use calendar[size].title. So we need a way for the Grommet theme to be accessible and change the Calendar heading level

@jcfilben jcfilben mentioned this pull request Dec 9, 2024
Merged
3 tasks
@britt6612 britt6612 requested a review from MikeKingdom December 9, 2024 23:57
@jcfilben jcfilben merged commit afc309c into grommet:master Dec 10, 2024
13 of 14 checks passed
This was referenced Jan 21, 2025
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MikeKingdom MikeKingdom MikeKingdom approved these changes

@jcfilben jcfilben jcfilben approved these changes

@taysea taysea Awaiting requested review from taysea

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Calendar - should be able to set the level of the header
4 participants
@britt6612 @jcfilben @taysea @MikeKingdom