Add `FieldLevelHelp` component by AparnaKarve · Pull Request #179 · patternfly/patternfly-react

AparnaKarve · 2018-01-23T23:40:41Z

Added a new component called FieldLevelHelp based on the design requirements in http://www.patternfly.org/pattern-library/forms-and-controls/help-on-forms/

FieldLevelHelp Storybook

Fixes #115

serenamarie125 · 2018-01-23T23:47:54Z

@AparnaKarve glad to see your first contribution here!

serenamarie125

@AparnaKarve couple comments re: UX review

#1 - can you change the placement of the popover so it "points" at the "I" icon as shown here in the design: http://www.patternfly.org/pattern-library/forms-and-controls/help-on-forms/#design

#2 - per the design, I think we only support with popover, not tooltip - @jgiardino what do you think? should we remove the tooltip option to be inline with the documentation and ensure consistency?

#3 - this is a nit, but wondering if you could change the example to bring up a new tab when the link in the popover is clicked ? That would be the desired behavior, so if it's easy to change it may be worth it

jeff-phillips-18 · 2018-01-24T00:19:52Z

Adding an action log for the link might be a better choice than adding a tab. Just my $00.02

jeff-phillips-18 · 2018-01-24T00:33:40Z

+    >
+      <label>
+        {children + ' '}
+        <Icon type="pf" name={'info'} style={{ color: '#0088ce' }} />


we shouldn't use an inline style here add the btn-link class instead.

btn-link introduces an underline that we typically see for links on a mouse hover, as shown below -

Is there a way to remove that?

dabeng · 2018-01-24T07:37:40Z

Hi @AparnaKarve , It will be better if you enable KNOBS panel to change "placement" or "trigger" properties 😊

ohadlevy · 2018-01-24T13:48:30Z

and please add tests :-)

AparnaKarve · 2018-01-24T19:26:59Z

@serenamarie125

#1 - can you change the placement of the popover so it "points" at the "I" icon as shown here in the design: http://www.patternfly.org/pattern-library/forms-and-controls/help-on-forms/#design

This is done. Can you please check the storybook again.

#2 - per the design, I think we only support with popover, not tooltip - @jgiardino what do you think? should we remove the tooltip option to be inline with the documentation and ensure consistency?

Let me know what we decide. IMO, tooltips are nice to have.

#3 - this is a nit, but wondering if you could change the example to bring up a new tab when the link in the popover is clicked ? That would be the desired behavior, so if it's easy to change it may be worth it

Addressed the above.

AparnaKarve · 2018-01-24T19:28:46Z

@jeff-phillips-18

Adding an action log for the link might be a better choice than adding a tab. Just my $00.02

The link is inside the popover. Not sure how to get the clicking action output in the action log.

AparnaKarve · 2018-01-24T19:36:05Z

It will be better if you enable KNOBS panel to change "placement" or "trigger" properties

@dabeng The knobs that are applicable in this case are mode and content and the Field Label itself. I have added those. Can you review the storybook once again?

AparnaKarve · 2018-01-24T19:56:09Z

@ohadlevy I have added some tests that pass in my environment - but seem to fail on Travis.

Not sure what the below CI error means -

New snapshot was not written. The update flag must be explicitly passed to write a new snapshot.
    
    This is likely because this test is run in a continuous integration (CI) environment in which snapshots are not written by default.

Did I miss something?

AparnaKarve · 2018-01-24T20:38:43Z

@priley86 @ohadlevy

Tests in this PR seem to be failing because of the snapshot files.

In the travis configuration, do we need to delete the snapshot files prior to npm run test?

I managed to recreate the CI failure locally, and it looks like running
./node_modules/.bin/jest --updateSnapshot before the tests execute, seems to fix it.

ohadlevy · 2018-01-24T20:43:06Z

@AparnaKarve you simply need to include the snapshot file too

AparnaKarve · 2018-01-24T20:49:09Z

Thanks @ohadlevy - that fixed it.

jeff-phillips-18 · 2018-01-24T22:53:46Z

I would agree that this particular component only be onClick per the design at http://www.patternfly.org/pattern-library/forms-and-controls/help-on-forms/#design

Tooltips are useful, but I think we want this component to follow the design to maintain consistency across applications using this.

ohadlevy

thanks @AparnaKarve I've left a few comments inline

ohadlevy · 2018-01-25T10:17:16Z

+
+  return (
+    <div>
+      <label>{children + ' '}</label>


nitpick: please write as:

<label>{`${children} `}</label>

Addressed in 2dd035d

ohadlevy · 2018-01-25T10:18:35Z

+    contentType === 'popover' ? (
+      <Popover id="{contentType}">{htmlContent}</Popover>
+    ) : (
+      <Tooltip id="{contentType}">{htmlContent}</Tooltip>


👍 to removing tooltip here, its not part of the pf-design.

Not removing the tooltip option in this iteration.
Keeping it temporarily for demo purposes, to explain an issue related to Formgroups and Popover below - #179 (comment)

ohadlevy · 2018-01-25T10:19:56Z

+import { defaultTemplate } from '../../../storybook/decorators/storyTemplates';
+import { DOCUMENTATION_URL } from '../../../storybook/constants';
+import { FieldLevelHelp } from './index';
+


maybe we should add it under the forms stories? it makes sense to me to see a complete form with field level help instead of a standalone?

In 6c9f1d2, I have added the story under Forms.

It was a good call to add the FieldLevelHelp use case in the Form, since it looks like I may have stumbled upon a bug in the Popover (or OverlayTrigger) component.

When the Popover component is used in conjunction with the FormGroup component, the Formgroup component steals the focus from Popover, causing the popover to close immediately. This happens in a fraction of a second, giving a visual impression that the popover never opened.

Please see a demo of the above issue here -
under the Forms->Horizontal Form->Phone field

For this particular use case, a workaround would be to keep the Popover open - (An option that you requested here - #179 (comment)). That way, we would at least see the popover contents.
The 'Close Popover' knob demonstrates the above (set the value to false)

The other workaround is to use Tooltips - but since we are not considering tooltips for the FieldLevelHelp component, that is probably not an option.
The 'Popover/Tooltip' knob demonstrates the above (set the value to 'tooltip')

ohadlevy · 2018-01-25T10:22:10Z

+    <FieldLevelHelp id="fieldlevelname1">Port Number</FieldLevelHelp>
+  );
+
+  let tree = component.toJSON();


I assume you can use const here?

Addressed in 2dd035d

ohadlevy · 2018-01-25T10:23:32Z

+  /** Contents displayed with popover or tooltip  */
+  content: PropTypes.string,
+  /** children nodes  */
+  children: PropTypes.node


does it make sense to have a props to force the popover to be open? (instead on just on hover/click)

Added a new prop - close, to close/open the popover if the use case needs it.
(if popover is forced to be open, it can be closed after clicking on the 'i' icon)

0db70f4

jgiardino · 2018-01-29T22:46:28Z

Thanks for adding this @AparnaKarve!

I also agree that this should only include the popover, and not the tooltip, to be consistent with the patternfly design documentation.

The only issue I see is with keyboard accessibility. I should be able to use the Tab key to place focus on the element that triggers the popover, and the Enter key to toggle the popover. Currently, the help icon is implemented as a <span> which can't receive keyboard focus. Alternatively, the bootstrap example uses a <button> and the patternfly example uses <a href="#">. (I think the <button> is more appropriate for this case, since we're not navigating anywhere.)

Also, how does this component fit in with the Forms component that currently exists? For example, will I be able to use the Forms component and include this FieldLevelHelp component? Or is additional work needed for either component to support this? And should we include a storybook that shows an example that uses both components? (We can create separate issues to track this work)

AparnaKarve · 2018-01-30T20:32:25Z

@jgiardino Thanks for the above feedback.

These are the most recent changes -

removed tooltips
implemented help icon as a button
(this seems to have resolved a focus issue that I was seeing earlier, mentioned in Add FieldLevelHelp component #179 (comment), so thanks for the tip)
added FieldLevelHelp in the Form stories

Let me know if you find anything else. Thanks.

jgiardino · 2018-01-30T22:22:21Z

+    >
+      <Button
+        bsStyle="link"
+        style={{ textDecoration: 'none', outline: 'none' }}


Can we remove these styles? Removing the outline results in an accessibility issue, because focus is not obvious to the sighted user who uses a keyboard to navigate.

Also, as a general rule, if styles are ever needed, chances are they should be added to the core patternfly repo so that all JS repos can benefit from them. There are probably rare cases where react has a unique case that exists only in the context of the react repo. In that case we would add .less and .scss files for the component. I just now opened PR #195 to the contributing guide that includes some of these updates, in case you want to review and provide feedback :-)

jgiardino · 2018-01-30T22:53:34Z

Yay, I'm glad using the button solved another issue. That was a happy accident!

I made a comment inline about inline styles. Related to this, I do realize that when the outline is visible, it's right up next to the text which is a little odd. But since there is no html implementation of this pattern in patternfly core that provides an example for us to follow, then I think for now what you have (after you remove the inline styles) is fine. I created an issue in patternfly to capture this: patternfly/patternfly#943

Thank you for updating the Form examples to show the inline help. When I was looking at the html for this example, I noticed some weirdness. The elements div.form-group and input.form-control are both getting attributes content and close:

<div content="Please specify Country code <br> <a target='_blank' href='https://countrycode.org/'>Click here for a list of Country codes</a>" close="true" class="form-group">
  <label for="phone" class="col-sm-3 control-label">
    Phone
    <button type="button" class="btn btn-link" style="text-decoration: none; outline: none;">
      <span aria-hidden="true" class="pficon pficon-info"></span>
    </button>
  </label>
  <div class="col-sm-9">
    <input type="phone" content="Please specify Country code <br> <a target='_blank' href='https://countrycode.org/'>Click here for a list of Country codes</a>" close="true" id="phone" class="form-control">
    <span class="help-block">Enter a valid phone number</span>
  </div>
</div>

AparnaKarve · 2018-01-31T01:02:55Z

@jgiardino I have removed the inline styling and have also addressed the html weirdness from the form stories #179 (comment)
Please take another look. Thanks.

jeff-phillips-18 · 2018-01-31T12:45:13Z

I know it's an issue in lots of places but the focus border on the info tip button encroaches on the label:

Can we set less horizontal padding and more horizontal margin to pull in the focus border a bit?

sharvit · 2018-01-31T14:22:29Z

@jeff-phillips-18, maybe it should just not implemented as a button? I think using a link, or just an icon can solve it without changing the styles (maybe just change the mouse pointer).

AparnaKarve · 2018-01-31T15:10:30Z

I think using a link, or just an icon can solve it without changing the styles (maybe just change the mouse pointer).

@sharvit My initial approach to this was just using an icon, but that had it's own set of issues.
One of the major ones being, the one described here - #179 (comment). This focus issue should still be viewable on my github.io fork here -
https://aparnakarve.github.io/patternfly-react
There are some knobs there to keep the popover open, which can be used as a workaround so that the Formgroup input does not steal focus from the open popover.

The other issue with the icon approach is what @jgiardino mentioned above, about keyboard accessibility.

jgiardino · 2018-01-31T15:24:15Z

Using <a> instead of <button> has no effect on the outline issue. Using one of these is necessary due to keyboard accessibility.

Regarding @jeff-phillips-18 suggestion, I'm fine with adding css to the pf-react repo to address the outline issue until pf core has an implementation that we can use. I would suggest something like this:
.field-help-pf: {outline-offset: -6px;}

With the button getting the class: <button class="btn btn-link pf-field-level" ...>

I pinged the css-ers under the patternfly group on slack for feedback about this too, so I may edit this in the near future if I get an alternate suggestion from them.

priley86 · 2018-02-06T19:50:48Z

one other thing i just now saw... your first commit needs to have commitizen syntax... so something like:

`feat(field-level-help): adds field level help to forms`

Also extracted out label from the component Adjusted Story and Tests accordingly

Also, displayed label independently and not as part of the FieldLevelHelp component

Styling to be addressed in patternfly/patternfly#943

keep the `dangerouslySetInnerHTML` usage confined to the stories

AparnaKarve · 2018-02-06T20:13:35Z

@priley86 I have reworded the first commit as you have indicated above.

priley86 · 2018-02-06T20:31:03Z

great job @AparnaKarve ... this looks great to me.

coveralls · 2018-02-06T20:34:47Z

Pull Request Test Coverage Report for Build 697

7 of 7 (100.0%) changed or added relevant lines in 1 file are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.1%) to 65.245%

Totals
Change from base Build 694:	0.1%
Covered Lines:	901
Relevant Lines:	1223

💛 - Coveralls

coveralls · 2018-02-06T20:34:47Z

Pull Request Test Coverage Report for Build 697

7 of 7 (100.0%) changed or added relevant lines in 1 file are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.1%) to 65.245%

Totals
Change from base Build 694:	0.1%
Covered Lines:	901
Relevant Lines:	1223

💛 - Coveralls

priley86 · 2018-02-06T20:40:06Z

woohoo 100% @AparnaKarve ! 🎉 🎉 thanks!

AparnaKarve · 2018-02-06T20:40:53Z

Thanks everyone for your valuable feedback!

serenamarie125 suggested changes Jan 23, 2018

View reviewed changes

jeff-phillips-18 reviewed Jan 24, 2018

View reviewed changes

AparnaKarve force-pushed the add_fieldlevelhelp_component branch from a33fc90 to 12fe00e Compare January 24, 2018 19:20

AparnaKarve force-pushed the add_fieldlevelhelp_component branch from 12fe00e to aeb861d Compare January 24, 2018 19:50

AparnaKarve force-pushed the add_fieldlevelhelp_component branch from aeb861d to fae5b63 Compare January 24, 2018 20:46

ohadlevy reviewed Jan 25, 2018

View reviewed changes

jgiardino reviewed Jan 30, 2018

View reviewed changes

jgiardino mentioned this pull request Jan 30, 2018

Inconsistency between HTML/CSS and React Implementation for field level help patternfly/patternfly-3#943

Closed

AparnaKarve force-pushed the add_fieldlevelhelp_component branch from 580ffd8 to 48995b8 Compare January 31, 2018 00:25

jeff-phillips-18 added the ux review label Jan 31, 2018

AparnaKarve added 16 commits February 6, 2018 12:02

feat(field-level-help): adds field level help to forms

bbfdaf7

Added tooltip support to FieldLevelHelp

6ac3dd7

Refactor to remove duplication

e4adcb8

Address lint errors

81d03bc

PR feedback - change placement of popover to point at the "i" icon

57dc1d3

Added knobs to demonstrate the FieldLevelHelp knobs

75cdd45

Basic rendering tests

8a2c17e

Rename contentType to mode

c7301d6

Address PR feedback

3392bcf

Added a new prop close to leave the popover open/close

4d840f5

Add FieldLevelHelp use case in the Form stories

33ab565

Removed tooltip support and changed Icon to Button type

e8973b6

Also extracted out label from the component Adjusted Story and Tests accordingly

Dropped tooltips from Form stories

e75e450

Also, displayed label independently and not as part of the FieldLevelHelp component

Remove inline styles on the component

fb84193

Styling to be addressed in patternfly/patternfly#943

Remove dangerouslySetInnerHTML from the component

d09fbd4

keep the `dangerouslySetInnerHTML` usage confined to the stories

Fix the label wording in the Stories

f5628db

AparnaKarve dismissed stale reviews from priley86 and jeff-phillips-18 via f5628db February 6, 2018 20:02

AparnaKarve force-pushed the add_fieldlevelhelp_component branch from 3f1a033 to f5628db Compare February 6, 2018 20:02

jeff-phillips-18 approved these changes Feb 6, 2018

View reviewed changes

priley86 approved these changes Feb 6, 2018

View reviewed changes

jeff-phillips-18 merged commit db4e1c1 into patternfly:master Feb 6, 2018

AparnaKarve deleted the add_fieldlevelhelp_component branch February 6, 2018 20:40

Conversation

AparnaKarve commented Jan 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

serenamarie125 commented Jan 23, 2018

Uh oh!

serenamarie125 left a comment

Choose a reason for hiding this comment

Uh oh!

jeff-phillips-18 commented Jan 24, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dabeng commented Jan 24, 2018

Uh oh!

ohadlevy commented Jan 24, 2018

Uh oh!

AparnaKarve commented Jan 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AparnaKarve commented Jan 24, 2018

Uh oh!

AparnaKarve commented Jan 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AparnaKarve commented Jan 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AparnaKarve commented Jan 24, 2018

Uh oh!

ohadlevy commented Jan 24, 2018

Uh oh!

AparnaKarve commented Jan 24, 2018

Uh oh!

jeff-phillips-18 commented Jan 24, 2018

Uh oh!

ohadlevy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AparnaKarve Jan 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jgiardino commented Jan 29, 2018

Uh oh!

AparnaKarve commented Jan 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jgiardino commented Jan 30, 2018

Uh oh!

AparnaKarve commented Jan 31, 2018

Uh oh!

jeff-phillips-18 commented Jan 31, 2018

Uh oh!

sharvit commented Jan 31, 2018

Uh oh!

AparnaKarve commented Jan 31, 2018

Uh oh!

AparnaKarve commented Jan 23, 2018 •

edited

Loading

AparnaKarve commented Jan 24, 2018 •

edited

Loading

AparnaKarve commented Jan 24, 2018 •

edited

Loading

AparnaKarve commented Jan 24, 2018 •

edited

Loading

AparnaKarve Jan 27, 2018 •

edited

Loading

AparnaKarve commented Jan 30, 2018 •

edited

Loading