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

add formField.checkBox.pad as an option to be handeled in formField #6300

Merged
merged 4 commits into from Aug 29, 2022

Conversation

britt6612
Copy link
Collaborator

@britt6612 britt6612 commented Aug 25, 2022

What does this PR do?

adds formField.checkBox.pad this will add pad to checkbox from theme.

Where should the reviewer start?

FormField

What testing has been done on this PR?

set pad value to formField.checkBox.pad

How should this be manually tested?

set pad value to formField.checkBox.pad
Custom story

Do Jest tests follow these best practices?

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

Any background context you want to provide?

What are the relevant issues?

needed for theme
grommet/grommet-theme-hpe#269

Screenshots (if appropriate)

Do the grommet docs need to be updated?

yes I will add a PR if this is the direction we go

Should this PR be mentioned in the release notes?

sure

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

backwards compatible

@britt6612 britt6612 requested a review from taysea August 25, 2022 18:39
@taysea
Copy link
Collaborator

taysea commented Aug 26, 2022

Can we add a unit test for this?

Copy link
Collaborator

@taysea taysea left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for adding the test. I tried the behavior locally and looks good.

@britt6612
Copy link
Collaborator Author

LGTM, thanks for adding the test. I tried the behavior locally and looks good.

thank you for the feedback 😄

@ericsoderberghp ericsoderberghp merged commit 95a5e83 into grommet:master Aug 29, 2022
kernicPanel added a commit to openfun/marsha that referenced this pull request Sep 5, 2022
Probably due to a bug in grommet v2.25.2, we must define a checkbox in
the formField in our theme.
grommet/grommet#6300
@@ -782,6 +782,9 @@ export interface ThemeType {
margin?: MarginType;
pad?: PadType;
};
checkBox: {
pad: PadType;
Copy link

Choose a reason for hiding this comment

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

Should not be optional like other keys ?

Copy link

Choose a reason for hiding this comment

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

Same for CheckBox key.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes you are correct pushing up a fix now sorry for any inconvenience. Thanks for the catch!

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.

None yet

4 participants