-
Notifications
You must be signed in to change notification settings - Fork 77
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
feat: upgrade fundamental-styles to 0.11.0-rc.4 #1082
Conversation
Deploy preview for fundamental-react ready! Built with commit c8143af |
LayoutPanel.Filters = LayoutPanelFilters; | ||
LayoutPanel.Footer = LayoutPanelFooter; | ||
LayoutPanel.Head = LayoutPanelHead; | ||
LayoutPanel.Header = LayoutPanelHeader; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've exported something wrong in layout panel but I can't figure out what found it
src/Select/Select.test.js
Outdated
@@ -51,7 +51,7 @@ describe('<Select />', () => { | |||
}); | |||
}); | |||
|
|||
test('forwards the ref to the button', () => { | |||
xtest('forwards the ref to the div role="button"', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@skvale I've done something wrong with how to handle forwarding the ref in Select - could you take a look when you get some time?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Popover is overriding the ref prop of this control element
let controlProps = {
onClick: onClickFunctions,
ref: (c) => {
this.controlRef = findDOMNode(c);
}
};
I'll see if there's way to extend the ref, but still maintain the original ref somehow
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to do something like this in Popover
let controlProps = {
onClick: onClickFunctions,
ref: (c) => {
this.controlRef = findDOMNode(c);
if (typeof control.ref === 'function') {
control.ref(c);
} else if (typeof control.ref === 'object') {
control.ref.current = c;
}
}
};
src/Select/Select.js
Outdated
require('fundamental-styles/dist/select.css'); | ||
} | ||
}, []); | ||
|
||
const divRef = ref ? ref : useRef(null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't conditionally call a hook
const internalDivRef = useRef(null);
const divRef = ref || internalDivRef;
src/StepInput/StepInput.js
Outdated
'is-disabled': disabled, | ||
'is-readonly': readOnly, | ||
[`is-${validationState?.state}`]: validationState ?.state |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder why eslint doesn't pick up on this line having an extra space validationState ?.state
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure - good catch though!
case 'space': | ||
e.stopPropagation(); | ||
handleSelect(e, option); | ||
setFocusedElement(tryFocus(divRef.current)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like the intent of the enter
and space
is to close the select. This isn't happening.
For some reason it is calling the handleClick
immediately after the handleSelect
and the handleClick
is toggling the isExpanded
state back to true
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It works as expected if you call e.preventDefualt();
though
@@ -1,60 +1,5 @@ | |||
// Jest Snapshot v1, https://goo.gl/fbAQLP | |||
|
|||
exports[`Storyshots Visual Button Group 1`] = ` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this was a duplicate of the same storyshot below
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Description
BREAKING CHANGES:
Panel
: renamedLayoutPanel
Select
: underlying html changesStepInput
: underlying html changesTile
: removedactive
propTile
:onClick
prop now required - all tiles are clickable by defaultTile
: removedproductTile
propTile
: removedtabIndex
prop (no longer necessary because they are always buttons now)Tile.Actions
removedTile.Media
removedTile.Content
no longer takes atitle
prop - this should now be added as a child toTile.Header
Tile.Content
: removedheadingLevel
propTile.Content
: removedtitleProps
propTile.Content
: removedproductTile
propTile.Content
: underlying html changes related to removal of titleFeatures:
List.Item
: addselected
stateSelect
: keyboard handling + focus handlingStepInput
: newcompact
stateTile
: newisDouble
prop (this is a bad name Help!) - maxes tile 2x1Tile
: newsize
propTile.Header
: new subcomponentTile.Footer
: new subcomponentTile.Content
: newtwoColumns
prop - enabled Content to be split into 2 columnsFixes:
fixes #1071