From e2d44523efa0f619411d50d8e0f14f861cce9df9 Mon Sep 17 00:00:00 2001 From: jongsue hong Date: Sat, 23 Feb 2019 00:17:25 +0900 Subject: [PATCH 1/7] fix(Dropdown): retains focus after selection --- src/lib/AutoControlledComponent.js | 5 +++-- src/modules/Dropdown/Dropdown.js | 16 ++++++++++------ test/specs/modules/Dropdown/Dropdown-test.js | 17 +++++++++++++++++ 3 files changed, 30 insertions(+), 8 deletions(-) diff --git a/src/lib/AutoControlledComponent.js b/src/lib/AutoControlledComponent.js index 8c48a6507e..eff5485b8c 100644 --- a/src/lib/AutoControlledComponent.js +++ b/src/lib/AutoControlledComponent.js @@ -183,8 +183,9 @@ export default class AutoControlledComponent extends Component { * Second argument is a state object that is always passed to setState. * @param {object} maybeState State that corresponds to controlled props. * @param {object} [state] Actual state, useful when you also need to setState. + * @param {function} [callback] a setState callback */ - trySetState = (maybeState, state) => { + trySetState = (maybeState, state, callback = () => {}) => { const { autoControlledProps } = this.constructor if (process.env.NODE_ENV !== 'production') { const { name } = this.constructor @@ -214,6 +215,6 @@ export default class AutoControlledComponent extends Component { if (state) newState = { ...newState, ...state } - if (Object.keys(newState).length > 0) this.setState(newState) + if (Object.keys(newState).length > 0) this.setState(newState, callback) } } diff --git a/src/modules/Dropdown/Dropdown.js b/src/modules/Dropdown/Dropdown.js index c4431e3a35..3ab4f3b136 100644 --- a/src/modules/Dropdown/Dropdown.js +++ b/src/modules/Dropdown/Dropdown.js @@ -470,7 +470,7 @@ export default class Dropdown extends Component { this.scrollSelectedItemIntoView() } else if (prevState.open && !this.state.open) { debug('dropdown closed') - this.handleClose() + // this.handleClose() } } @@ -489,7 +489,7 @@ export default class Dropdown extends Component { const { closeOnChange, multiple } = this.props const shouldClose = _.isUndefined(closeOnChange) ? !multiple : closeOnChange - if (shouldClose) this.close(e) + if (shouldClose) this.close(e, () => {}) } closeOnEscape = (e) => { @@ -695,7 +695,11 @@ export default class Dropdown extends Component { // Notify the onAddItem prop if this is a new value if (isAdditionItem) _.invoke(this.props, 'onAddItem', e, { ...this.props, value }) - if (search) _.invoke(this.searchRef, 'focus') + if (search) { + _.invoke(this.searchRef, 'focus') + } else { + _.invoke(this.ref, 'focus') + } } handleFocus = (e) => { @@ -1127,13 +1131,13 @@ export default class Dropdown extends Component { this.scrollSelectedItemIntoView() } - close = (e) => { + close = (e, callback = this.handleClose) => { const { open } = this.state debug('close()', { open }) if (open) { _.invoke(this.props, 'onClose', e, this.props) - this.trySetState({ open: false }) + this.trySetState({ open: false }, undefined, callback) } } @@ -1144,7 +1148,7 @@ export default class Dropdown extends Component { // https://github.com/Semantic-Org/Semantic-UI-React/issues/627 // Blur the Dropdown on close so it is blurred after selecting an item. // This is to prevent it from re-opening when switching tabs after selecting an item. - if (!hasSearchFocus) { + if (!hasSearchFocus && this.ref) { this.ref.blur() } diff --git a/test/specs/modules/Dropdown/Dropdown-test.js b/test/specs/modules/Dropdown/Dropdown-test.js index 7f9f88883f..98e66e28ef 100644 --- a/test/specs/modules/Dropdown/Dropdown-test.js +++ b/test/specs/modules/Dropdown/Dropdown-test.js @@ -1261,6 +1261,7 @@ describe('Dropdown', () => { dropdownMenuIsOpen() // select item + item.simulate('mousedown') item.simulate('click') dropdownMenuIsClosed() }) @@ -2137,6 +2138,22 @@ describe('Dropdown', () => { `Expected "input.search" to be the active element but found ${activeElement} instead.`, ) }) + + it('sets focus to the dropdown after selection', () => { + const randomIndex = _.random(options.length - 1) + + wrapperMount() + .simulate('click', nativeEvent) + .find('DropdownItem') + .at(randomIndex) + .simulate('click', nativeEvent) + + const activeElement = document.activeElement + const dropdownIsFocused = activeElement === document.querySelector('div.dropdown') + dropdownIsFocused.should.be.true( + `Expected Dropdown to be the active element but found ${activeElement} instead.`, + ) + }) }) describe('searchInput', () => { From be1eccd35a0ecaf4db21ab9edbace52b91e79240 Mon Sep 17 00:00:00 2001 From: jongsue hong Date: Wed, 13 Mar 2019 23:00:14 +0900 Subject: [PATCH 2/7] remove the state argument in trySetState method to match React's API --- src/lib/AutoControlledComponent.js | 26 ++----------------- src/modules/Dropdown/Dropdown.js | 4 +-- src/modules/Rating/Rating.js | 2 +- src/modules/Search/Search.js | 2 +- .../specs/lib/AutoControlledComponent-test.js | 11 -------- 5 files changed, 6 insertions(+), 39 deletions(-) diff --git a/src/lib/AutoControlledComponent.js b/src/lib/AutoControlledComponent.js index eff5485b8c..df3fab7a2e 100644 --- a/src/lib/AutoControlledComponent.js +++ b/src/lib/AutoControlledComponent.js @@ -182,39 +182,17 @@ export default class AutoControlledComponent extends Component { * Safely attempt to set state for props that might be controlled by the user. * Second argument is a state object that is always passed to setState. * @param {object} maybeState State that corresponds to controlled props. - * @param {object} [state] Actual state, useful when you also need to setState. * @param {function} [callback] a setState callback */ - trySetState = (maybeState, state, callback = () => {}) => { - const { autoControlledProps } = this.constructor - if (process.env.NODE_ENV !== 'production') { - const { name } = this.constructor - // warn about failed attempts to setState for keys not listed in autoControlledProps - const illegalKeys = _.difference(_.keys(maybeState), autoControlledProps) - if (!_.isEmpty(illegalKeys)) { - console.error( - [ - `${name} called trySetState() with controlled props: "${illegalKeys}".`, - 'State will not be set.', - 'Only props in static autoControlledProps will be set on state.', - ].join(' '), - ) - } - } - - let newState = Object.keys(maybeState).reduce((acc, prop) => { + trySetState = (maybeState, callback = () => {}) => { + const newState = Object.keys(maybeState).reduce((acc, prop) => { // ignore props defined by the parent if (this.props[prop] !== undefined) return acc - // ignore props not listed in auto controlled props - if (autoControlledProps.indexOf(prop) === -1) return acc - acc[prop] = maybeState[prop] return acc }, {}) - if (state) newState = { ...newState, ...state } - if (Object.keys(newState).length > 0) this.setState(newState, callback) } } diff --git a/src/modules/Dropdown/Dropdown.js b/src/modules/Dropdown/Dropdown.js index 49ba48770d..636a748ab9 100644 --- a/src/modules/Dropdown/Dropdown.js +++ b/src/modules/Dropdown/Dropdown.js @@ -758,7 +758,7 @@ export default class Dropdown extends Component { const newQuery = value _.invoke(this.props, 'onSearchChange', e, { ...this.props, searchQuery: newQuery }) - this.trySetState({ searchQuery: newQuery }, { selectedIndex: 0 }) + this.trySetState({ searchQuery: newQuery, selectedIndex: 0 }) // open search dropdown on search query if (!open && newQuery.length >= minCharacters) { @@ -1138,7 +1138,7 @@ export default class Dropdown extends Component { if (open) { _.invoke(this.props, 'onClose', e, this.props) - this.trySetState({ open: false }, undefined, callback) + this.trySetState({ open: false }, callback) } } diff --git a/src/modules/Rating/Rating.js b/src/modules/Rating/Rating.js index b50c6ab6d4..2c6bc32778 100644 --- a/src/modules/Rating/Rating.js +++ b/src/modules/Rating/Rating.js @@ -83,7 +83,7 @@ export default class Rating extends Component { } // set rating - this.trySetState({ rating: newRating }, { isSelecting: false }) + this.trySetState({ rating: newRating, isSelecting: false }) if (onRate) onRate(e, { ...this.props, rating: newRating }) } diff --git a/src/modules/Search/Search.js b/src/modules/Search/Search.js index 4d0421770c..58710e41a8 100644 --- a/src/modules/Search/Search.js +++ b/src/modules/Search/Search.js @@ -448,7 +448,7 @@ export default class Search extends Component { const { selectFirstResult } = this.props - this.trySetState({ value }, { selectedIndex: selectFirstResult ? 0 : -1 }) + this.trySetState({ value, selectedIndex: selectFirstResult ? 0 : -1 }) } moveSelectionBy = (e, offset) => { diff --git a/test/specs/lib/AutoControlledComponent-test.js b/test/specs/lib/AutoControlledComponent-test.js index 001b5ba3c2..5752351b90 100644 --- a/test/specs/lib/AutoControlledComponent-test.js +++ b/test/specs/lib/AutoControlledComponent-test.js @@ -65,17 +65,6 @@ describe('extending AutoControlledComponent', () => { wrapper.should.have.state(randomProp, randomValue) }) - it('does not set state for non autoControlledProps', () => { - consoleUtil.disableOnce() - - TestClass = createTestClass({ autoControlledProps: [], state: {} }) - const wrapper = shallow() - - wrapper.instance().trySetState({ [faker.hacker.noun()]: faker.hacker.verb() }) - - wrapper.state().should.be.empty() - }) - it('does not set state for props defined by the parent', () => { consoleUtil.disableOnce() From 54af9ec7662eb05c936c599193969da51f18deb4 Mon Sep 17 00:00:00 2001 From: jongsue hong Date: Thu, 14 Mar 2019 00:51:52 +0900 Subject: [PATCH 3/7] add assertions for 'focus' state --- test/specs/modules/Dropdown/Dropdown-test.js | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/test/specs/modules/Dropdown/Dropdown-test.js b/test/specs/modules/Dropdown/Dropdown-test.js index 9f2e05a065..02a8180929 100644 --- a/test/specs/modules/Dropdown/Dropdown-test.js +++ b/test/specs/modules/Dropdown/Dropdown-test.js @@ -2148,21 +2148,23 @@ describe('Dropdown', () => { }) it('sets focus to the search input after selection', () => { - wrapperMount() - // random item, skip the first as it's selected by default const randomIndex = 1 + _.random(options.length - 2) - wrapper + wrapperMount() + .simulate('click', nativeEvent) .find('DropdownItem') .at(randomIndex) - .simulate('click') + .simulate('click', nativeEvent) + + dropdownMenuIsClosed() const activeElement = document.activeElement const searchIsFocused = activeElement === document.querySelector('input.search') searchIsFocused.should.be.true( `Expected "input.search" to be the active element but found ${activeElement} instead.`, ) + wrapper.should.have.state('focus', true) }) it('sets focus to the dropdown after selection', () => { @@ -2174,11 +2176,15 @@ describe('Dropdown', () => { .at(randomIndex) .simulate('click', nativeEvent) + dropdownMenuIsClosed() + const activeElement = document.activeElement const dropdownIsFocused = activeElement === document.querySelector('div.dropdown') dropdownIsFocused.should.be.true( `Expected Dropdown to be the active element but found ${activeElement} instead.`, ) + + wrapper.should.have.state('focus', true) }) }) From c8d3b5191a020392d320a57b54ac3d7dde960040 Mon Sep 17 00:00:00 2001 From: jongsue hong Date: Thu, 14 Mar 2019 00:53:10 +0900 Subject: [PATCH 4/7] update Dropdown.js --- src/modules/Dropdown/Dropdown.js | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/modules/Dropdown/Dropdown.js b/src/modules/Dropdown/Dropdown.js index 636a748ab9..6264f014b7 100644 --- a/src/modules/Dropdown/Dropdown.js +++ b/src/modules/Dropdown/Dropdown.js @@ -475,7 +475,6 @@ export default class Dropdown extends Component { this.scrollSelectedItemIntoView() } else if (prevState.open && !this.state.open) { debug('dropdown closed') - // this.handleClose() } } @@ -700,17 +699,18 @@ export default class Dropdown extends Component { } this.clearSearchQuery() - this.closeOnChange(e) - - // Heads up! This event handler should be called after `onChange` - // Notify the onAddItem prop if this is a new value - if (isAdditionItem) _.invoke(this.props, 'onAddItem', e, { ...this.props, value }) if (search) { _.invoke(this.searchRef.current, 'focus') } else { - _.invoke(this.ref, 'focus') + _.invoke(this.ref.current, 'focus') } + + this.closeOnChange(e) + + // Heads up! This event handler should be called after `onChange` + // Notify the onAddItem prop if this is a new value + if (isAdditionItem) _.invoke(this.props, 'onAddItem', e, { ...this.props, value }) } handleFocus = (e) => { @@ -1149,7 +1149,7 @@ export default class Dropdown extends Component { // https://github.com/Semantic-Org/Semantic-UI-React/issues/627 // Blur the Dropdown on close so it is blurred after selecting an item. // This is to prevent it from re-opening when switching tabs after selecting an item. - if (!hasSearchFocus && this.ref) { + if (!hasSearchFocus && this.ref.current) { this.ref.current.blur() } From e6b83b350cda29331b6c86d66306b1065760e23c Mon Sep 17 00:00:00 2001 From: Oleksandr Fediashov Date: Sun, 4 Aug 2019 12:39:53 +0200 Subject: [PATCH 5/7] restore style --- .../src/components/CarbonAd/CarbonAdNative.js | 31 +++++++++---------- .../ComponentTable/ComponentTableRow.js | 2 +- docs/src/components/LayoutsLayout.js | 3 +- .../Portal/Types/PortalExamplePortal.js | 31 ++++--------------- .../Radio/States/RadioExampleRemoteControl.js | 8 ++--- .../Usage/ResponsiveExampleOnUpdate.js | 13 ++------ .../TransitionablePortalExampleControlled.js | 13 ++------ .../TransitionablePortalExampleTransition.js | 30 +++--------------- 8 files changed, 36 insertions(+), 95 deletions(-) diff --git a/docs/src/components/CarbonAd/CarbonAdNative.js b/docs/src/components/CarbonAd/CarbonAdNative.js index 2f747554cb..2f298a6728 100644 --- a/docs/src/components/CarbonAd/CarbonAdNative.js +++ b/docs/src/components/CarbonAd/CarbonAdNative.js @@ -67,8 +67,8 @@ class CarbonAdNative extends PureComponent { debug('handleNativeJSON', { mounted: this.mounted }) try { const sanitizedAd = json.ads - .filter((ad) => Object.keys(ad).length > 0) - .filter((ad) => !!ad.statlink) + .filter(ad => Object.keys(ad).length > 0) + .filter(ad => !!ad.statlink) .filter(Boolean)[0] debug('handleNativeJSON sanitizedAd', sanitizedAd) @@ -99,19 +99,19 @@ class CarbonAdNative extends PureComponent { const colors = inverted ? { - divider: '#333', - background: '#222', - backgroundHover: '#1d1d1d', - color: '#999', - colorHover: '#ccc', - } + divider: '#333', + background: '#222', + backgroundHover: '#1d1d1d', + color: '#999', + colorHover: '#ccc', + } : { - divider: '#eee', - background: '#fff', - backgroundHover: 'whitesmoke', - color: '#555', - colorHover: '#333', - } + divider: '#eee', + background: '#fff', + backgroundHover: 'whitesmoke', + color: '#555', + colorHover: '#333', + } return ( @@ -142,8 +142,7 @@ class CarbonAdNative extends PureComponent { /> ))} -