feat(FormControls): updated logic for rendering icons #9132

thatblindgeye · 2023-05-16T20:05:02Z

What: Closes #9023

@srambach @mcarrano currently the Core examples for Form Control shows a Textinput with both a custom icon and a status icon. I put this functionality in React's TextInput, but notTextArea or FormSelect. Do we want to allow multiple icons to be rendered, and if so should it be on all 3 of these components?

Other notes:

deprecated Select and TreeViewSearch were both using inputs with the pf-v5-c-form-control class. These both (or just TreeView?) will end up needing to be updated to use SearchInput instead. For now the styling for TreeViewSearch is different as I hardcoded in markup to render the search icon on the right (rather than the left).
The autoResize logic in TextArea currently does not work
Added a FromControlIcon helper component rather than adding a ___Icon sub-component for TextArea,FormSelect, and TextInput. I can move this to a more applicable directory if need be.

Additional issues:

patternfly-build · 2023-05-16T20:15:28Z

Preview: https://patternfly-react-pr-9132.surge.sh

A11y report: https://patternfly-react-pr-9132-a11y.surge.sh

nicolethoen · 2023-05-17T12:23:05Z

And this will need a code mod, right?

props being removed related to iconSprites
props being changed related to customIcons
the TextInput's isReadOnly prop being removed
the new way to handle expandable text areas
probably more I missed?

nicolethoen · 2023-05-17T12:24:32Z

The autoResize logic in TextArea currently does not work

@thatblindgeye is this a regression? Is it because of the new DOM structure? Can we fix it w/o a breaking change and maybe make a follow up issue or does it require investigation today?

thatblindgeye · 2023-05-17T12:43:01Z

@nicolethoen Yeah there'll definitely be some codemods needed after this update. I'm going to go through and open an issue(s) for anything I catch.

And I believe it might be due to new structure, but I'm going to investigate a bit more into it.

srambach · 2023-05-17T12:48:50Z

Do we want to allow multiple icons to be rendered, and if so should it be on all 3 of these components?

I don't know of any place that uses an icon (not a status icon) on a text area or select. @mcoker?
The text input definitely needs to support both a custom icon and a status.

The autoResize logic in TextArea currently does not work

This is almost definitely due to the new structure - in order to make sure the icon doesn't float off by itself, the resize was moved out onto the wrapper div rather than the text area itself. Then the text area just takes up as much room as there is. Is there a way to manage auto resize with that?

thatblindgeye · 2023-05-17T13:20:27Z

@nicolethoen @srambach Updated the autoresizing logic. Looks like the previous heightToken isn't actually applied to anything in Core anymore, so now just setting the height on the .pf-v5-c-form-control div.

srambach · 2023-05-17T14:39:14Z

File upload changed a little

mcarrano

@thatblindgeye this looks mostly good but I did identify some issue. Not sure if these result from this PR or other changes that were made.

The header area in the File Upload component is weirdly truncating.

For number input, the input fields now have spinners on them, they did not before.

For the Time Picker, looks like the icon used inside of the field changed. PF4 was using the outlined clock which was per the original design.

mcoker · 2023-05-17T15:18:02Z

I don't know of any place that uses an icon (not a status icon) on a text area or select. @mcoker?
The text input definitely needs to support both a custom icon and a status.

@srambach @thatblindgeye I'd say let's just support whatever was supported in v4. Applying a custom icon to both select and textarea don't work as expected, so I think we can just keep that feature on textinput

mcoker · 2023-05-17T15:49:17Z

@mcarrano @srambach @thatblindgeye Looks like we're styling the old form control class for things that need to be updated with to style the right element based on the updates made in core to remove the background images. That will account for some of these visual bugs you're seeing @mcarrano. I created an issue to fix those - patternfly/patternfly#5583

thatblindgeye · 2023-05-17T16:56:29Z

@mcarrano I updated the icon on TimePicker. Looks like the other issues should be covered by the issue @mcoker created in Core above.

nicolethoen

the react code here looks solid

mcarrano

@mcarrano I updated the icon on TimePicker. Looks like the other issues should be covered by the issue @mcoker created in Core above.

In that case I approve.

tlabaj · 2023-05-17T19:59:55Z

packages/react-core/src/helpers/FormControlIcon.tsx

@@ -0,0 +1,31 @@
+import * as React from 'react';


I don't know that I would put this component in helpers. I would recommend adding it with rest of our components. Just don't export it.

tlabaj · 2023-05-17T20:16:16Z

packages/react-core/src/helpers/FormControlIcon.tsx

+export const FormControlIcon = ({ status, customIcon, className, ...props }: FormControlIconProps) => {
+  const StatusIcon = status && statusIcons[status];
+
+  return customIcon || StatusIcon ? (


I do not think this check is necessary this is an internal component and will always be set.

tlabaj · 2023-05-18T00:11:24Z

@mcarrano @srambach @thatblindgeye Looks like we're styling the old form control class for things that need to be updated with to style the right element based on the updates made in core to remove the background images. That will account for some of these visual bugs you're seeing @mcarrano. I created an issue to fix those - patternfly/patternfly#5583

@mcoker is this good to merge as is or do do we want to wait for core bug to be fixed?

mcoker · 2023-05-18T01:22:42Z

@tlabaj no need to wait for core IMO, those updates will not need a react follow up.

mcoker

LGTM! Just one note about a modifier on <TextInput> that I don't think is necessary.

mcoker · 2023-05-18T15:30:28Z

packages/react-core/src/components/TextInput/TextInput.tsx

+          isDisabled && styles.modifiers.disabled,
+          isExpanded && styles.modifiers.expanded,
+          customIcon && styles.modifiers.icon,
+          placeholder && styles.modifiers.placeholder,


This doesn't appear to be necessary, and it looks incorrect in the core docs, too. We style the placeholder (update the color) via ::placeholder (meaning the html element has a placeholder attribute) - https://github.com/patternfly/patternfly/blob/474883c523da6bf7b5bbc44dc415b2d15429565c/src/patternfly/components/FormControl/form-control.scss#L150-L152

We use .pf-m-placeholder on <FormSelect> because a <select> is different and doesn't support placeholder. https://github.com/patternfly/patternfly/blob/474883c523da6bf7b5bbc44dc415b2d15429565c/src/patternfly/components/FormControl/form-control.scss#L277

tlabaj

LGTM!

patternfly-build · 2023-05-18T18:11:15Z

Your changes have been released in:

@patternfly/react-charts@7.0.0-alpha.27
@patternfly/react-code-editor@5.0.0-alpha.110
@patternfly/react-core@5.0.0-alpha.109
@patternfly/react-docs@6.0.0-alpha.117
@patternfly/react-icons@5.0.0-alpha.16
demo-app-ts@5.0.0-alpha.93
@patternfly/react-integration@5.0.0-alpha.47
@patternfly/react-styles@5.0.0-alpha.12
@patternfly/react-table@5.0.0-alpha.111
@patternfly/react-tokens@5.0.0-alpha.11

Thanks for your contribution! 🎉

* feat(FormControls): updated logic for rendering icons * Updated FormSelect * Updated TextArea * Updated treeview search markup * Updated remaining integration tests * Updated autoresize logic * Updated snaphots * Updated timepicker icon * Updated FormControlIcon placement and rendering * Removed placeholder class on Textinput * Updated snapshots * Updated snapshots properly

thatblindgeye changed the title ~~Iss9023 form control icons~~ feat(FormControls): updated logic for rendering icons May 16, 2023

thatblindgeye requested a review from srambach May 16, 2023 20:05

thatblindgeye added the css review label May 16, 2023

thatblindgeye requested a review from mcarrano May 16, 2023 21:03

thatblindgeye added the ux review label May 16, 2023

wise-king-sullyman mentioned this pull request May 16, 2023

feat(Table): added class names for table elements #9133

Merged

thatblindgeye force-pushed the iss9023_formControlIcons branch from 2bbcc98 to 08d1ba3 Compare May 17, 2023 13:18

thatblindgeye mentioned this pull request May 17, 2023

Form controls - updated props and markup patternfly/pf-codemods#475

Closed

mcarrano requested changes May 17, 2023

View reviewed changes

thatblindgeye force-pushed the iss9023_formControlIcons branch from 19b6b9b to c651a2a Compare May 17, 2023 16:53

nicolethoen approved these changes May 17, 2023

View reviewed changes

mcarrano approved these changes May 17, 2023

View reviewed changes

wise-king-sullyman approved these changes May 17, 2023

View reviewed changes

thatblindgeye force-pushed the iss9023_formControlIcons branch from c651a2a to d7be8b8 Compare May 17, 2023 18:56

tlabaj requested changes May 17, 2023

View reviewed changes

thatblindgeye force-pushed the iss9023_formControlIcons branch from d7be8b8 to 3c3731e Compare May 17, 2023 22:27

thatblindgeye requested a review from tlabaj May 17, 2023 22:27

gitdallas approved these changes May 17, 2023

View reviewed changes

tlabaj mentioned this pull request May 18, 2023

feat(Flex): add plain gap spacing #9143

Merged

kmcfaul approved these changes May 18, 2023

View reviewed changes

mcoker approved these changes May 18, 2023

View reviewed changes

thatblindgeye added 9 commits May 18, 2023 11:40

feat(FormControls): updated logic for rendering icons

4b331d9

Updated FormSelect

16e8039

Updated TextArea

befcfd8

Updated treeview search markup

90507c3

Updated remaining integration tests

fcbac4e

Updated autoresize logic

a8d554c

Updated snaphots

c8d8c8e

Updated timepicker icon

c342b25

Updated FormControlIcon placement and rendering

bddb156

thatblindgeye force-pushed the iss9023_formControlIcons branch from 3c3731e to bddb156 Compare May 18, 2023 15:46

thatblindgeye added 3 commits May 18, 2023 11:48

Removed placeholder class on Textinput

8bf1097

Updated snapshots

484d280

Updated snapshots properly

2f8fd09

tlabaj approved these changes May 18, 2023

View reviewed changes

tlabaj merged commit 9f4863f into patternfly:v5 May 18, 2023
10 checks passed

thatblindgeye mentioned this pull request May 24, 2023

fix(form-control): update form-control in other components patternfly/patternfly#5609

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(FormControls): updated logic for rendering icons #9132

feat(FormControls): updated logic for rendering icons #9132

thatblindgeye commented May 16, 2023 •

edited

patternfly-build commented May 16, 2023 •

edited

nicolethoen commented May 17, 2023

nicolethoen commented May 17, 2023

thatblindgeye commented May 17, 2023

srambach commented May 17, 2023 •

edited

thatblindgeye commented May 17, 2023

srambach commented May 17, 2023 •

edited

mcarrano left a comment

mcoker commented May 17, 2023

mcoker commented May 17, 2023 •

edited

thatblindgeye commented May 17, 2023

nicolethoen left a comment

mcarrano left a comment

tlabaj May 17, 2023

tlabaj May 17, 2023 •

edited

tlabaj commented May 18, 2023

mcoker commented May 18, 2023

mcoker left a comment

mcoker May 18, 2023

tlabaj left a comment

patternfly-build commented May 18, 2023

feat(FormControls): updated logic for rendering icons #9132

feat(FormControls): updated logic for rendering icons #9132

Conversation

thatblindgeye commented May 16, 2023 • edited

patternfly-build commented May 16, 2023 • edited

nicolethoen commented May 17, 2023

nicolethoen commented May 17, 2023

thatblindgeye commented May 17, 2023

srambach commented May 17, 2023 • edited

thatblindgeye commented May 17, 2023

srambach commented May 17, 2023 • edited

mcarrano left a comment

Choose a reason for hiding this comment

mcoker commented May 17, 2023

mcoker commented May 17, 2023 • edited

thatblindgeye commented May 17, 2023

nicolethoen left a comment

Choose a reason for hiding this comment

mcarrano left a comment

Choose a reason for hiding this comment

tlabaj May 17, 2023

Choose a reason for hiding this comment

tlabaj May 17, 2023 • edited

Choose a reason for hiding this comment

tlabaj commented May 18, 2023

mcoker commented May 18, 2023

mcoker left a comment

Choose a reason for hiding this comment

mcoker May 18, 2023

Choose a reason for hiding this comment

tlabaj left a comment

Choose a reason for hiding this comment

patternfly-build commented May 18, 2023

thatblindgeye commented May 16, 2023 •

edited

patternfly-build commented May 16, 2023 •

edited

srambach commented May 17, 2023 •

edited

srambach commented May 17, 2023 •

edited

mcoker commented May 17, 2023 •

edited

tlabaj May 17, 2023 •

edited