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

Routine maintenance 2023-08 #596

Merged
merged 22 commits into from
Aug 22, 2023
Merged

Routine maintenance 2023-08 #596

merged 22 commits into from
Aug 22, 2023

Conversation

chawes13
Copy link
Contributor

@chawes13 chawes13 commented Aug 2, 2023

QA

Out of Scope

Author Checklist

  • Add unit test(s)
  • Update version in package.json (see the versioning guidelines)
  • Update documentation (if necessary)
  • Add story to storybook (if necessary)
  • Assign dev reviewer

@@ -23,9 +23,13 @@ function DropdownSelect({ children, className, selectedValues }) {
return (
<OutsideClickHandler onOutsideClick={() => toggleExpanded(false)}>
<div className="dropdown-select">
<div className="select-input" onClick={() => toggleExpanded()}>
<button
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Flagged by the jsx-a11y plugin from upgrading lpl's eslint-config

Comment on lines +13 to +14
* - `errorComponent`
* - `labelComponent`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Noticed these were missing from the docs

Comment on lines +67 to +68
previewComponent: PropTypes.func, // eslint-disable-line react/no-unused-prop-types
children: PropTypes.node, // eslint-disable-line react/no-unused-prop-types
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We know these are being used by the RenderPreview component

@@ -44,7 +44,7 @@ import { compose, generateInputErrorId } from '../../utils'

const propTypes = {
...fieldPropTypesWithValue(PropTypes.bool),
label: PropTypes.node,
label: PropTypes.node, // eslint-disable-line react/no-unused-prop-types
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We know this is getting passed to LabeledField

@chawes13
Copy link
Contributor Author

chawes13 commented Aug 2, 2023

@bhennes2 I didn't originally intend for this to be a breaking change, but once I upgraded to our latest eslint configuration there were a few jsx-a11y errors that I couldn't ignore. Overall the changes are all styling based on probably don't affect too many projects (if any)!

@chawes13 chawes13 requested a review from bhennes2 August 2, 2023 20:53
Comment on lines -14 to -20
test('ColorPicker collapses when background is clicked', () => {
const wrapper = mount(<ColorPicker />)
wrapper.find('.swatch').simulate('click')
expect(wrapper.find('.popover').exists()).toBe(true)
wrapper.find('.cover').simulate('click')
expect(wrapper.find('.popover').exists()).toBe(false)
})
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Enzyme's click simulation doesn't trigger the OnOutsideClick component's logic. I'm ok with that, given that we're using a third party library which has its own test coverage

@chawes13
Copy link
Contributor Author

@bhennes2 Bump 🤜

Copy link

@bhennes2 bhennes2 left a comment

Choose a reason for hiding this comment

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

Overall looking good - have a few follow up questions on the omitLabelProps changes just the make sure I'm understanding the diff between props and rest.

.storybook/styles/components/_tabs.scss Show resolved Hide resolved
src/controls/tab-bar/tab-bar.js Show resolved Hide resolved
test/controls/tab-bar.test.js Show resolved Hide resolved
src/forms/inputs/checkbox-group.js Show resolved Hide resolved
src/forms/inputs/checkbox-group.js Show resolved Hide resolved
@bhennes2 bhennes2 self-requested a review August 18, 2023 21:34
Copy link

@bhennes2 bhennes2 left a comment

Choose a reason for hiding this comment

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

I believe we cleared up my remaining questions so going to re-review and approve!

@chawes13 chawes13 mentioned this pull request Aug 22, 2023
1 task
@chawes13 chawes13 merged commit f5146b3 into main Aug 22, 2023
1 check passed
@chawes13 chawes13 deleted the chore/maintenance-2023-08 branch August 22, 2023 20:17
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.

Dependency Dashboard Console warning about duplicate manual mock Upgrade Jest to v28
2 participants