fix: add disable edge detection to popover #546
Conversation
|
Deploy preview for fundamental-react ready! Built with commit a1bbef0 |
jbadan
left a comment
jbadan
left a comment
There was a problem hiding this comment.
I think here: https://github.com/SAP/fundamental-react/blob/master/src/Popover/Popover.js#L102 would be a good place to add a link to the Popper.js documentation.
Other than my few housekeeping changes this is looking really good!!
src/Popover/Popover.test.js
Outdated
| // popover with no disable edge detection | ||
| component = renderer.create(popOverDisableEdgeDetection); | ||
| tree = component.toJSON(); | ||
| expect(tree).toMatchSnapshot(); |
There was a problem hiding this comment.
I'm not sure if this actually tests anything. I can't think of a useful unit test for this, perhaps it's time to bite the bullet and set up puppeteer? Thoughts @greg-a-smith ?
There was a problem hiding this comment.
I agree that this test doesn't really do anything. The only way to visually test this prop is to open the popover and scroll to a point where it should flip and then verify it doesn't.
Since that functionality is really core to popper.js, we could probably get away with a test that makes sure our internal Popper component gets this value passed to it and the modifiers are set correctly. Let me know if you need/want more explanation on that.
src/TimePicker/TimePicker.js
Outdated
|
|
||
| TimePicker.propDescriptions = { | ||
| ...Time.propDescriptions, | ||
| popoverProps: 'Additional props to be spread to the [Popover](/popover) component.', |
There was a problem hiding this comment.
Do we have a default for this? If not, we should add one so we don't have to rewrite it in all the components.
There was a problem hiding this comment.
See previous note about using defaults.js.
src/ComboboxInput/ComboboxInput.js
Outdated
| ComboboxInput.propDescriptions = { | ||
| menu: 'An object containing a `Menu` component.' | ||
| menu: 'An object containing a `Menu` component.', | ||
| popoverProps: 'Additional props to be spread to the [Popover](/popover) component.' |
There was a problem hiding this comment.
This actually already exists in the defaults.js file albeit with "Popover" marked in backticks rather than a link. I think making it a link is a good idea, but then it should be edited in defaults.js so all components get the same change.
| language: 'Text to display on the `<button>` element. Meant to be the language of the text in the `<input>`/`<textarea>` element.' | ||
| }, | ||
| menu: 'An array of objects that represent the values of the elements in the dropdown menu. The shape of the objects in the array is `{ placeholder: string, language: string, inputProps: object }`.', | ||
| popoverProps: 'Additional props to be spread to the [Popover](/popover) component.', |
There was a problem hiding this comment.
See previous note about using defaults.js.
src/MultiInput/MultiInput.js
Outdated
| localizedTextShape: { | ||
| imageLabel: 'Aria-label in <div> element for image.' | ||
| }, | ||
| popoverProps: 'Additional props to be spread to the [Popover](/popover) component.', |
There was a problem hiding this comment.
See previous note about using defaults.js.
src/Popover/Popover.Component.js
Outdated
| <Popover | ||
| body={bodyContent} | ||
| control={<Button glyph='navigation-down-arrow' option='light' />} | ||
| disableEdgeDetection='true' |
There was a problem hiding this comment.
Same comment about boolean props.
src/Popover/Popover.test.js
Outdated
| // popover with no disable edge detection | ||
| component = renderer.create(popOverDisableEdgeDetection); | ||
| tree = component.toJSON(); | ||
| expect(tree).toMatchSnapshot(); |
There was a problem hiding this comment.
I agree that this test doesn't really do anything. The only way to visually test this prop is to open the popover and scroll to a point where it should flip and then verify it doesn't.
Since that functionality is really core to popper.js, we could probably get away with a test that makes sure our internal Popper component gets this value passed to it and the modifiers are set correctly. Let me know if you need/want more explanation on that.
src/TimePicker/TimePicker.js
Outdated
|
|
||
| TimePicker.propDescriptions = { | ||
| ...Time.propDescriptions, | ||
| popoverProps: 'Additional props to be spread to the [Popover](/popover) component.', |
There was a problem hiding this comment.
See previous note about using defaults.js.
jbadan
left a comment
jbadan
left a comment
There was a problem hiding this comment.
Everything looks great to me! 🎉🚢
src/Popover/Popover.js
Outdated
| noArrow: 'Set to **true** to render a popover without an arrow.', | ||
| placement: 'Initial position of the `body` (overlay) related to the `control`.', | ||
| popperProps: 'Additional props to be spread to the overlay element.', | ||
| popperProps: 'Additional props to be spread to the overlay element, supported by <a href="https://popper.js.org" target="blank">popper.js</a>.', |
There was a problem hiding this comment.
Although it appears to work, the correct target value is _blank.
Description
fixes #540