feat: normalize FormValidationOverlay usage in other components #1183
Conversation
|
Deploy preview for fundamental-react ready! Built with commit 1396409 |
| @@ -143,8 +143,15 @@ Please ensure you are either using a visible \`FormLabel\` or an \`aria-label\` | |||
| labelProps: PropTypes.object, | |||
| /** Sets the `name` for the checkbox input */ | |||
| name: PropTypes.string, | |||
| /** Additional classes to apply to validation popover */ | |||
| validationClassName: PropTypes.string, | |||
There was a problem hiding this comment.
Should moving this prop to validationOverlayProps.className be listed as a breaking change as well?
| document.body.innerHTML = ''; | ||
| }); | ||
|
|
||
| const getFormMessage = () => document.body.querySelector('.fd-popover__popper > div > .fd-form-message'); |
There was a problem hiding this comment.
Just an idea or an alternative, I've seen tests that include
// Ensure the popper is accessible with enzyme
jest.mock('react-dom', () => {
const original = jest.requireActual('react-dom');
return {
...original,
createPortal: (node) => node,
};
});so you can still access the elements via enzyme
There was a problem hiding this comment.
I like this, but I'd like to apply it globally in config/jest/setup.js so we don't have some unit tests with it/some without. It makes 450 unit tests fail at the moment so I'll probably skip it on this pr 😆
|
|
||
| const messageNode = getFormMessage(); | ||
|
|
||
| expect(messageNode).toBeDefined(); |
There was a problem hiding this comment.
I'm pretty sure expect(null).toBeDefined() passes. If querySelector can't find an element it returns null, so this expect will always pass.
There was a problem hiding this comment.
Well... crap haha bad unit test!
There was a problem hiding this comment.
The one after this is going to have the same problem - I'll rework both
| }); | ||
| }); | ||
|
|
||
| describe('Interactions', () => { |
There was a problem hiding this comment.
@skvale Looks like you solved this exact problem in tooltip in widgets recently - I copied and applied what you did there to get this working 🎉
|
I forgot to merge this as a breaking change 😞 |
|
@jbadan any chance we would use |
|
@bcullman We could try... but that prop was added 1 day before and removed 1 day after. We're most likely the only ones that noticed it came and went 😆 |
Description
validationOverlayPropsto all components that consumeFormValidationOverlay:Checkbox,FormInput,FormTextArea,Select,InputGroup,SearchInput,Select,StepInputvalidationOverlayPropin all consuming componentsvalidationOverlayProp.formMessagePropsalso spread toFormMessagewhen used inside component (outside ofValidationOverlay- seeSelectorSearchInput). This way, if you are expectingFormMessageto receive additional classes, it will whether it's inside the dropdown or as it's own popover. This will be useful in nui-widgets-lab for override Gateway theming colors._FormValidationOverlay(not exported to consumers) now spreads additional props to the wrapping<div>element viawrapperProps. I couldn't use...restbecause that creates a rabbithole of problems with the wayPopoveris cloning elements. Possibly worth a followup, but is a ton of work and creates a pretty confusing output just for continuity sake.BREAKING CHANGES:
fd-list--has-messageclass from Select's dropdown - it was adding unnecessary padding. It was a relic from before FormMessage rendered inside a PopovervalidationClassNamerenamed and moved inside ofvalidationOverlayProps, use likevalidationOverlayProps.classNameThere will be at least 2 additional prs for Select - 1 to add the ability to pass custom classes to each component, the second addressing #1159