feat: update fundamental-styles to 0.12.0-rc.44 #1203
Conversation
|
Deploy preview for fundamental-react ready! Built with commit 410e943 |
| /> | ||
| > | ||
| <i | ||
| className="sap-icon--navigation-left-arrow" |
There was a problem hiding this comment.
Do all these icons need aria-hidden attributes like we do in widgets?
src/Select/Select.js
Outdated
| {!readOnly && <span className={triggerClassNames} />} | ||
| {!readOnly && | ||
| <span className={triggerClassNames}> | ||
| <i className='sap-icon--slim-arrow-down' /> |
There was a problem hiding this comment.
Can this use the Icon component + does it need some sort of aria-label?
src/Button/Button.js
Outdated
| {children} | ||
| {iconBeforeText && glyph && <i aria-hidden='true' className={iconClasses} />} | ||
| {children && <span className={buttonTextClasses}>{children}</span>} | ||
| {!iconBeforeText && glyph && <i aria-hidden='true' className={iconClasses} />} |
There was a problem hiding this comment.
Same here, can these use the Icon component + remove import of Icon.css?
src/Icon/Icon.js
Outdated
| @@ -22,9 +22,13 @@ const Icon = React.forwardRef(({ glyph, size, className, ...props }, ref) => { | |||
| style = { fontSize: ICON_SIZES[size] }; | |||
| } | |||
|
|
|||
| const Tag = isInButton ? 'i' : 'span'; | |||
There was a problem hiding this comment.
I was hesitant to use the Icon component because it uses a span instead of i. Should we go back to fundamental-styles so that all icons use an i as in nui-widgets?
There was a problem hiding this comment.
After the discussion this morning, I think they're in the process of moving all icons to their own <i> tag- SAP/fundamental-styles#1604
We can probably just go ahead and use i tags since it doesn't cause any styling issues and then styles will catch up. Up to you though
There was a problem hiding this comment.
👍 If we could use the Icon component and use i tags to get ahead of it where possible that would save us time down the line.
| @@ -53,7 +53,7 @@ export const types = () => ( | |||
|
|
|||
| types.storyName = 'Types'; | |||
|
|
|||
| /** Button can have an icon with text or just and icon. */ | |||
| /** Button can have an icon with text or just an icon. Be sure to use an `aria-label` if there is no text. */ | |||
|
|
|||
There was a problem hiding this comment.
Can we add aria-label to the
<Button glyph='alert' option='emphasized' />in this file.
| ariaHidden | ||
| glyph={glyph} /> | ||
| ) : null; | ||
|
|
There was a problem hiding this comment.
Does it work to just do something like:
const renderIcon = () => {
return ( glyph && <Icon props here />
)
}
There was a problem hiding this comment.
lint complains Declare only one React component per file
Not sure if it's cleaner to disable it / if we want to 🤷♂️
There was a problem hiding this comment.
Eh, I think it's fine the way you have it
Description
BREAKING CHANGE
<i>tag in button<i>tag inIconand button text moved into a<span>with class ‘fd-button__text’iconBeforeText,iconProps, andtextClassNameprop toButtonariaHiddenandariaLabelprops toIcon