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

496 fieldset #506

Merged
merged 7 commits into from
Oct 3, 2022
Merged

496 fieldset #506

merged 7 commits into from
Oct 3, 2022

Conversation

BCerki
Copy link
Contributor

@BCerki BCerki commented Sep 22, 2022

Fixes #496

Proposed Changes

@BCerki BCerki force-pushed the 491-alert branch 2 times, most recently from d198128 to 42f6ba5 Compare September 22, 2022 21:54
@BCerki BCerki changed the base branch from 491-alert to develop September 23, 2022 20:20
@BCerki BCerki marked this pull request as ready for review September 23, 2022 20:20
@BCerki BCerki requested a review from junminahn as a code owner September 23, 2022 20:20
Copy link
Contributor

@tmastrom tmastrom left a comment

Choose a reason for hiding this comment

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

Let's discuss testing and stories. I added some comments however the testing may be more relevant on the base component.

const results = await axe(container);

expect(results).toHaveNoViolations();
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add testing for passing the user props? There are some examples in the bcgov Input component that could be useful.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added this test to the component-library test rather than the button one. All the Button components only have the accessibility test so I'm guessing the idea was to thoroughly test the base components and then only add Button tests if the component is significantly different than the base one. If that seems like a problem I can make a tech debt card to come up with a testing strategy/team agreement

},
required: {
description: 'Indicates whether the field is required.',
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we add the fullWidth prop here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm going to make a tech debt card to look at this in other components--I suspect there are other stories that are missing props too

tmastrom
tmastrom previously approved these changes Oct 3, 2022
type: 'select',
options: ['small', 'medium', 'large'],
},
description: 'The size of the checkbox',
Copy link
Contributor

Choose a reason for hiding this comment

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

"input field" instead of checkbox?

size: 'medium',
},
staticProps: ['fullWidth'],
forwardProps: ['size', 'variant', 'disabled', 'required', 'fullWidth'],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The fullWidth version has some ugly spacing, but since we're going to need to do proper styling once we have designs anyway, I think it's okay for now

@tmastrom tmastrom merged commit 2269103 into develop Oct 3, 2022
@tmastrom tmastrom deleted the 496-fieldset branch October 3, 2022 22:11
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.

Add Fieldset component to Button theme
2 participants