feat: upgrade fundamental-styles to 0.11.0-rc.100 #1121

jacobdevera · 2020-07-09T22:34:50Z

Description

Affected Components

Shellbar: classes updated (buttons should have focus outline); remove ‘fd-product-menu’ class
Select: role changed to combobox, aria-haspopup changed to listbox, fixing aria-disabled and aria-readonly
Table row selection styling
Checkbox: fd-checkbox__text class added for ellipsis overflow
Dialog: remove backdropClassName, remove overlay classes
DatePicker: fd-bar—cosy renamed to fd-bar—cozy; new icon for ‘choose date’ button
Icon: sizes removed, using font-size
Popover: is-expanded class added

netlify · 2020-07-09T22:37:10Z

Deploy preview for fundamental-react ready!

Built with commit 00de77f

https://deploy-preview-1121--fundamental-react.netlify.app

jacobdevera · 2020-07-09T22:38:05Z

src/utils/constants.js

+export const ICON_SIZES = {
+    's': '12px',
+    'm': '16px',
+    'l': '20px',
+    'xl': '24px'
+};


Debating removing these like what was done for styles, but for some reason it feels wrong to not have predefined sizes and having the consumer set it.

I think icon sizes should not have been removed from fundamental-styles - I think this is fine until they add them back

Fiori 3 doesn't have predefined sizes - the icon size follows the font-size
the values should be in rem at least:

0.75rem 1rem 1.25rem 1.5rem

skvale · 2020-07-10T13:29:49Z

src/Table/Table.js

                        displayRows = row.rowData.splice(1, row.rowData.length);
                    }

                    return (
                        <tr
                            className={tableRowClasses}
                            {...rowProps}
+                            aria-selected={selectedRows[index]}


If we know the first item in the row is a checkbox, can we just do

aria-selected={row?.rowData?[0]?.props.checked}

Otherwise we'd have duplicated selected logic here and for the consumer, in this case Table.stories.js. They could get out of sync. Such as if they wanted to render a row pre-selected this component would not account for that.

skvale · 2020-07-10T13:33:51Z

src/Forms/Checkbox.js

@@ -77,7 +83,7 @@ const Checkbox = React.forwardRef(({
                className={labelClasses}
                disabled={disabled}
                htmlFor={checkId}>
-                {children}
+                {checkboxChildren}


on a related note, is children a required prop? I saw this warning in our Table stories

Warning: Failed prop type: The prop `children` is marked as required in `Checkbox`, but its value is `undefined`. in Checkbox (created by storyFn) in storyFn in ErrorBoundary

I think it was me that set it to required - from my memory it was to enforce the checkbox having a label

Unless they change the design for the table with checkbox, we need to mark children as not required and provide an aria-label alternative. Or do a custom prop so we require either children or an aria-label.

jbadan · 2020-07-10T14:16:50Z

src/Dialog/Dialog.js

@@ -161,8 +152,6 @@ Dialog.propTypes = {
    actions: PropTypes.arrayOf(PropTypes.node).isRequired,
    /** Localized text for the heading */
    title: PropTypes.string.isRequired,
-    /** CSS class(es) to add to the dialog backdrop */
-    backdropClassName: PropTypes.string,


I think we should still keep the ability to put a class on the backdrop even though fd-overlay was removed

jbadan · 2020-07-10T14:19:58Z

src/Icon/Icon.js

@@ -34,7 +34,7 @@ Icon.propTypes = {
    /** CSS class(es) to add to the element */
    className: PropTypes.string,
    /** Size of the component: 's', 'm', 'l', 'xl' */
-    size: PropTypes.oneOf(ICON_SIZES)
+    size: PropTypes.oneOf(Object.keys(ICON_SIZES))


let's choose a default size

Actually let's not so it just gets the whatever font-size - can we make that prop conditional so we don't have undefined when it's not specified though

I'm a little confused here—the markup seems to be fine when size is undefined (i.e. no inline styling applied) and there's no error. Is a conditional check still necessary?

Look at the diff for InputGroup:

<span className="sap-icon--globe" style={ Object { "fontSize": undefined, } } />

It should probably not even add the style prop if fontSize is undefined instead of adding that

jbadan · 2020-07-10T14:23:15Z

src/SearchInput/__stories__/__snapshots__/SearchInput.stories.storyshot

@@ -158,6 +157,7 @@ exports[`Storyshots Component API/SearchInput Disabled 1`] = `
              >
                <div
                  className="fd-input-group--control fd-input-group"
+                  onClick={[Function]}


Is it right for the onClick to be on a different element than the focus and blur?

The FormValidationOverlay has focus and blur events for showing the validation message. The onClick is from the Popover adding click handling and other attributes to the control component; these props used to be spread to the overlay's base div. But this would be problematic because then the overlay would get all the aria attributes. So I thought it made more sense for FormValidationOverlay to pass down all props spread to it to the control component instead, as if there was no overlay. Not sure if there is a case for spreading props to it.

Either way, it works the same.

jbadan

Huge undertaking!!! All my concerns have been addressed ⛵

jacobdevera added 19 commits July 7, 2020 10:22

fix: rc 47, shellbar classnames

06d95c0

fix: rc 69, table selection styling

6c399d0

fix: rc 71, remove overlay

04777ee

fix: rc 73, cozy

d12832e

chore: rc 74, remove product menu

d1e9495

chore: rc 75, datepicker button icon

55be7aa

chore: rc 88, snapshots

a247004

feat: rc 89, removing icon sizes, removed classes

e32235f

fix: icon size

f60e9f7

chore: rc 93, snapshots

356d9bf

fix: test

b1f0dde

fix: test

545e1a6

feat: rc 95 checkbox label ellipsis overflow

c06cc26

fix: rc 97, select combobox role

28f8d9f

fix: checkbox text fix, select a11y fixes

6e2f871

fix: simplify props passing

c28da3c

fix: duplicate id in select and popper

ba0c029

fix: test validation state overriding focus announcement

41892a0

chore: rc 100, storyshots

56164d5

jacobdevera requested review from meganaconley and a team July 9, 2020 22:34

jacobdevera self-assigned this Jul 9, 2020

jacobdevera commented Jul 9, 2020

View reviewed changes

skvale reviewed Jul 10, 2020

View reviewed changes

jbadan reviewed Jul 10, 2020

View reviewed changes

jacobdevera added 7 commits July 10, 2020 10:56

fix: remove duplicate checked state

f4a1320

fix: aria-label Checkbox, select all rows example

bc1dd60

fix: header cell checked

8da44bd

fix: add back backdropClassName

d051e4a

fix: select all rows checked

209c069

fix: use rem values

19b9710

fix: conditonal style prop

00de77f

jbadan approved these changes Jul 14, 2020

View reviewed changes

skvale approved these changes Jul 14, 2020

View reviewed changes

jacobdevera merged commit 695aac4 into master Jul 14, 2020

jacobdevera deleted the feat/styles-upgrade branch July 14, 2020 16:26

jbadan mentioned this pull request Sep 1, 2020

Dialog: improve a11y #1030

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: upgrade fundamental-styles to 0.11.0-rc.100 #1121

feat: upgrade fundamental-styles to 0.11.0-rc.100 #1121

jacobdevera commented Jul 9, 2020

netlify bot commented Jul 9, 2020 •

edited

Loading

jacobdevera Jul 9, 2020

jbadan Jul 10, 2020

droshev Jul 10, 2020 •

edited

Loading

skvale Jul 10, 2020 •

edited

Loading

skvale Jul 10, 2020 •

edited

Loading

jbadan Jul 10, 2020

meganaconley Jul 10, 2020

jbadan Jul 10, 2020

jbadan Jul 10, 2020

jbadan Jul 10, 2020

jacobdevera Jul 10, 2020

jbadan Jul 13, 2020

jbadan Jul 10, 2020

jacobdevera Jul 10, 2020

jbadan left a comment

feat: upgrade fundamental-styles to 0.11.0-rc.100 #1121

feat: upgrade fundamental-styles to 0.11.0-rc.100 #1121

Conversation

jacobdevera commented Jul 9, 2020

Description

Affected Components

netlify bot commented Jul 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

droshev Jul 10, 2020 • edited Loading

Choose a reason for hiding this comment

skvale Jul 10, 2020 • edited Loading

Choose a reason for hiding this comment

skvale Jul 10, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbadan left a comment

Choose a reason for hiding this comment

netlify bot commented Jul 9, 2020 •

edited

Loading

droshev Jul 10, 2020 •

edited

Loading

skvale Jul 10, 2020 •

edited

Loading

skvale Jul 10, 2020 •

edited

Loading