feat(Dropdown): add `openOnFocus` and `closeOnBlur` #1101

tarang9211 · 2016-12-31T12:27:40Z

Fixes #517

tarang9211 · 2016-12-31T12:31:29Z

@levithomason This PR seems a little vague as of now since I don't have a 100% understanding of the exact issue. But what I did was added the two props to the Dropdown.js file. Let me know what additional changes to make and I'll be more than happy to implement and learn!

codecov-io · 2016-12-31T12:33:17Z

Current coverage is 95.87% (diff: 100%)

No coverage report found for master at 75be304.

Powered by Codecov. Last update 75be304...23f389f

levithomason · 2016-12-31T17:12:24Z

src/modules/Dropdown/Dropdown.js

@@ -296,6 +296,10 @@ export default class Dropdown extends Component {
    /** The text displayed in the dropdown, usually for the active item. */
    text: PropTypes.string,

+    openOnFocus: PropTypes.bool,


Let's add some descriptions to these:

"Whether or not the menu should open when the dropdown is focused."

And:

"Whether or not the menu should close when the dropdown is blurred."

levithomason · 2016-12-31T17:22:25Z

Now, the open method needs to return early if openOnFocus is false. Similar to how it currently exits early if disabled.

The same is true for the close method and closeOnBlur.

Both of these new props should be added to defaultProps and set to true. This way, the current behavior is preserved.

We should also have two new doc examples, one for each prop. These would go in the "Usage" section.

Finally, we'll add some tests after all this is done.

tarang9211 · 2017-01-01T09:03:24Z

So, what's the difference between the menu and the dropdown? The menu is in the dropdown?

So basically open the dropdown when the user focusses it by tabbing to it?

And close the dropdown when you tab away?

… and closeOnBlur props

tarang9211 · 2017-01-02T19:21:10Z

@levithomason let me know how this PR looks as of now. Time permitting, could you clarify my questions above?

levithomason · 2017-01-03T20:27:32Z

So, what's the difference between the menu and the dropdown? The menu is in the dropdown?

Correct.

So basically open the dropdown when the user focusses it by tabbing to it?
And close the dropdown when you tab away?

Exactly. These features already exist, however, they are always "on" and there is no way to turn them off. This PR is adding the ability to turn them off.

On this note, the logic should actually be added to the componentDidUpdate where it is calling open/close on focus/blur:

https://github.com/tarang9211/Semantic-UI-React/blob/35bd319e09132096c6fa3f2844c4f29bf36e9005/src/modules/Dropdown/Dropdown.js#L405

https://github.com/tarang9211/Semantic-UI-React/blob/35bd319e09132096c6fa3f2844c4f29bf36e9005/src/modules/Dropdown/Dropdown.js#L419

tarang9211 · 2017-01-03T20:30:27Z

So the logic added to the open & close methods currently is incorrect?

levithomason · 2017-01-03T21:24:06Z

Indeed, my mistake on the initial instructions. The goal is to allow the user to enable/disable this behavior. My suggestion would be to get the doc examples written and wired up, then play with them until you feel it is working correctly. I can then review the progress.

I thought I'd have more time to dive into this and give guidance, however, it would help me if you could push some further attempts at it on your own and I'll try to help guide you.

tarang9211 · 2017-01-03T21:26:57Z

No worries. Hope my questions weren't too silly! I'll have some example for you ready in a few hours.

tarang9211 · 2017-01-09T13:04:41Z

@levithomason Hi! I've made the changes and added example in the last three commits. Please could you review these for me? 💯

levithomason

Just had a new 👶 so might be a little slow on follow up, but I managed to get some reviews in on this. We're getting real close, thanks much!

levithomason · 2017-01-09T23:31:57Z

docs/app/Examples/modules/Dropdown/Usage/DropdownExampleOpenOnFocus.js

+import { friendOptions } from '../common'
+
+const DropdownExampleOpenOnFocus = () => (
+  <Dropdown placeholder='Select Friend' openOnFocus fluid selection options={friendOptions} />


Let's show both cases here as well.

levithomason · 2017-01-09T23:33:06Z

docs/app/Examples/modules/Dropdown/Usage/DropdownExampleCloseOnBlur.js

+import { friendOptions } from '../common'
+
+const DropdownExampleCloseOnBlur = () => (
+  <Dropdown placeholder='Select Friend' closeOnBlur fluid selection options={friendOptions} />


How about we show both true/false cases for comparison?

<div> <Dropdown closeOnBlur options={friendOptions} placeholder='I close on blur' selection /> {' '} <Dropdown closeOnBlur={false} options={friendOptions} placeholder='I stay open on blur' selection /> </div>

levithomason · 2017-01-09T23:40:00Z

src/modules/Dropdown/Dropdown.js

        debug('mouse is not down, opening')
+        if (!openOnFocus) return


Small syntactical change request: if (openOnFocus) this.open(), since there is an explicit 1:1 correlation between the condition and the action in this case.

levithomason · 2017-01-09T23:40:14Z

src/modules/Dropdown/Dropdown.js

        debug('mouse is not down, closing')
+        if (!closeOnBlur) return


Same here: if (closeOnBlur) this.close().

…penOnFocus and closeOnBlue props to docs site

tarang9211 · 2017-01-10T06:50:56Z

@levithomason Congratulations on the 👶! Hope he/she is doing well!

i've made the changes in the last commit. I also have a .mov file that I created to show how it's working currently. Don't think Github supports that, where would be the best place to send you that file?

I also see this error when I use the arrow keys to move through the menu items. Here's a screenshot of that specific error.

levithomason · 2017-01-11T18:02:49Z

docs/app/Examples/modules/Dropdown/Usage/DropdownExampleOpenOnFocus.js

@@ -3,7 +3,11 @@ import { Dropdown } from 'semantic-ui-react'
 import { friendOptions } from '../common'

 const DropdownExampleOpenOnFocus = () => (
-  <Dropdown placeholder='Select Friend' openOnFocus fluid selection options={friendOptions} />
+  <div>
+    <Dropdown placeholder='I stay open on focus' openOnFocus fluid selection options={friendOptions} />


-I stay open on focus +I open on focus

levithomason · 2017-01-11T18:03:20Z

docs/app/Examples/modules/Dropdown/Usage/DropdownExampleOpenOnFocus.js

+  <div>
+    <Dropdown placeholder='I stay open on focus' openOnFocus fluid selection options={friendOptions} />
+    <br />
+    <Dropdown placeholder='I close when not focussed' openOnFocus={false} fluid selection options={friendOptions} />


-I close when not focussed +I do not open on focus

levithomason · 2017-01-11T18:04:07Z

docs/app/Examples/modules/Dropdown/Usage/DropdownExampleCloseOnBlur.js

-  <Dropdown placeholder='Select Friend' closeOnBlur fluid selection options={friendOptions} />
+  <div>
+    <Dropdown placeholder='I close on blur' closeOnBlur fluid selection options={friendOptions} />
+    <br />


Let's drop the fluid prop and go with {' '} here so these are inline elements rather than full page vertically stacked elements. Just helps keep the docs more concise.

levithomason · 2017-01-11T18:04:43Z

Thanks for the updates, I'll pull this branch down and give it a go as soon as I am able. I left a couple more comments for now 👍 Thanks again.

…d better placeholders

tarang9211 · 2017-01-11T19:18:55Z

@levithomason Done! Just made the changes you suggested.

tarang9211 · 2017-01-11T19:31:27Z

src/modules/Dropdown/Dropdown.js

        debug('mouse is not down, closing')
-        this.close()
+        if (closeOnBlur) this.close()


Does this line look fine to you, logically?

Alright, hmm. I'm seeing something weird. When I go to the dropdown with closeOnBlur being true, and tab out of it to the adjacent one, the first one closes. It shouldn't right?

tarang9211 · 2017-01-11T19:31:35Z

src/modules/Dropdown/Dropdown.js

        debug('mouse is not down, opening')
-        this.open()
+        if (openOnFocus) this.open()


Does this line look fine to you, logically?

levithomason · 2017-01-11T20:04:14Z

Perfect thank you!

tarang9211 · 2017-01-11T20:10:53Z

@levithomason what about tests? 😸

levithomason · 2017-01-11T20:18:33Z

Released in semantic-ui-react@0.63.6.

Yes, tests would be great. Didn't want to hold this one up too much longer for you, since there are tests covering the basic use already. If you want to start a new PR for that, we just need to copy some of the existing tests and update then to work for this case 👍

tarang9211 · 2017-01-11T20:20:38Z

Cool, any example tests I could look at for this? The other test case was kind of easy since I just had to check if the component had the class name appended to it. Not the case for this one, right?

levithomason · 2017-01-11T20:22:39Z

We add a describe() block for each prop, so describe('openOnFocus', () => { ... }) for example. Then you can find other focus/blur tests and use those for inspiration. Example:

https://github.com/Semantic-Org/Semantic-UI-React/blob/master/test/specs/modules/Dropdown/Dropdown-test.js#L150

tarang9211 · 2017-01-11T20:25:58Z

Sounds good. Did you get a chance to review this comment?

Alright, hmm. I'm seeing something weird. When I go to the dropdown with closeOnBlur being true, and tab out of it to the adjacent one, the first one closes. It shouldn't right?

levithomason · 2017-01-11T20:26:35Z

Just beat me to it! Was about to comment the same. I checked the source as well, not sure right off hand why that is. Let's investigate and fix it on the next PR.

tarang9211 · 2017-01-11T20:28:07Z

wait a sec, shouldn't it be an if else statement. And in the else, just do this.open()?

mull · 2017-01-19T15:58:38Z

src/modules/Dropdown/Dropdown.js

@@ -409,8 +418,9 @@ export default class Dropdown extends Component {
    } else if (prevState.focus && !this.state.focus) {
      debug('dropdown blurred')
      if (!this.isMouseDown) {


Hey guys I know I'm late to the party. I wanted to discuss this line with you. So I'm doing a search Dropdown and I don't want it to open on focus. This works fine while tabbing through the form, but clicking on a form field is also focusing on it, yet it'll still open.

It makes sense that the menu opens if you click on the arrow, but I think for search it's a bit unintuitive if it opens when one clicks on the input.

Thoughts?

feat(Dropdown): added two more props to Dropdown component

510afda

levithomason reviewed Dec 31, 2016

View reviewed changes

levithomason added the needs author feedback label Dec 31, 2016

tarang9211 added 4 commits January 1, 2017 17:24

feat(Dropdown): added descriptions for openOnFocus and closeOnBlur props

a0a5963

feat(Dropdown): added closeOnBlur and openOnFocus to default props

e23854e

feat(Dropdown): fixed minor eslint error

69aeee2

feat(Dropdown): added logic to open & close functions for openOnFocus…

35bd319

… and closeOnBlur props

tarang9211 added 3 commits January 9, 2017 00:33

feat(Dropdown): modified componentDidUpdate to handle new prop behavior

33fcd85

feat(Dropdown): added example for openOnFocus prop to docs site

eac7c8f

feat(Dropdown): added closeOnBlur example to Dropdown docs site

f08de41

levithomason requested changes Jan 9, 2017

View reviewed changes

feat(Dropdown): fixed minor syntax changes, added more examples for o…

23f389f

…penOnFocus and closeOnBlue props to docs site

levithomason requested changes Jan 11, 2017

View reviewed changes

levithomason removed the needs author feedback label Jan 11, 2017

feat(Dropdown): removed fluid props from dropdown docs site, and adde…

b002a52

…d better placeholders

tarang9211 commented Jan 11, 2017

View reviewed changes

levithomason approved these changes Jan 11, 2017

View reviewed changes

levithomason changed the title ~~feat(Dropdown): added two more props to Dropdown component~~ feat(Dropdown): add openOnFocus and closeOnBlur Jan 11, 2017

levithomason merged commit d6cdabb into Semantic-Org:master Jan 11, 2017

mull reviewed Jan 19, 2017

View reviewed changes

feat(Dropdown): add openOnFocus and closeOnBlur #1101

feat(Dropdown): add openOnFocus and closeOnBlur #1101

Conversation

tarang9211 commented Dec 31, 2016 • edited by levithomason Loading

tarang9211 commented Dec 31, 2016

codecov-io commented Dec 31, 2016 • edited Loading

Current coverage is 95.87% (diff: 100%)

Choose a reason for hiding this comment

levithomason commented Dec 31, 2016 • edited Loading

tarang9211 commented Jan 1, 2017 • edited Loading

tarang9211 commented Jan 2, 2017

levithomason commented Jan 3, 2017

tarang9211 commented Jan 3, 2017

levithomason commented Jan 3, 2017

tarang9211 commented Jan 3, 2017

tarang9211 commented Jan 9, 2017

levithomason left a 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

tarang9211 commented Jan 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

levithomason commented Jan 11, 2017

tarang9211 commented Jan 11, 2017

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

levithomason commented Jan 11, 2017

tarang9211 commented Jan 11, 2017

levithomason commented Jan 11, 2017

tarang9211 commented Jan 11, 2017

levithomason commented Jan 11, 2017

tarang9211 commented Jan 11, 2017

levithomason commented Jan 11, 2017

tarang9211 commented Jan 11, 2017

Choose a reason for hiding this comment

feat(Dropdown): add `openOnFocus` and `closeOnBlur` #1101

feat(Dropdown): add `openOnFocus` and `closeOnBlur` #1101

tarang9211 commented Dec 31, 2016 •

edited by levithomason

Loading

codecov-io commented Dec 31, 2016 •

edited

Loading

levithomason commented Dec 31, 2016 •

edited

Loading

tarang9211 commented Jan 1, 2017 •

edited

Loading

tarang9211 commented Jan 10, 2017 •

edited

Loading