New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix button link modal not closing #3508

Merged
merged 2 commits into from Nov 21, 2017

Conversation

Projects
None yet
2 participants
@vladanost
Contributor

vladanost commented Nov 15, 2017

Description

Fixes #3501

Button link suggestion modal could only be closed buy first using arrow keys and pressing ENTER afterwards. When clicked, the link changed but the modal was stuck.

How Has This Been Tested?

JS unit test passing

Types of changes

I added a new method to handle this.

@codecov

This comment has been minimized.

Show comment
Hide comment
@codecov

codecov bot Nov 15, 2017

Codecov Report

Merging #3508 into master will increase coverage by 0.67%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3508      +/-   ##
==========================================
+ Coverage   34.61%   35.28%   +0.67%     
==========================================
  Files         261      267       +6     
  Lines        6769     6745      -24     
  Branches     1231     1221      -10     
==========================================
+ Hits         2343     2380      +37     
+ Misses       3733     3688      -45     
+ Partials      693      677      -16
Impacted Files Coverage Δ
blocks/url-input/index.js 3.17% <0%> (-0.06%) ⬇️
blocks/library/shortcode/index.js 40% <0%> (-10%) ⬇️
blocks/library/verse/index.js 28.57% <0%> (-8.93%) ⬇️
blocks/library/code/index.js 50% <0%> (-7.15%) ⬇️
blocks/library/html/index.js 16.66% <0%> (-6.42%) ⬇️
blocks/library/preformatted/index.js 44.44% <0%> (-5.56%) ⬇️
blocks/library/table/index.js 36.36% <0%> (-5.31%) ⬇️
blocks/library/text-columns/index.js 28.57% <0%> (-4.77%) ⬇️
blocks/library/video/index.js 16.66% <0%> (-4.39%) ⬇️
blocks/api/validation.js 91.46% <0%> (-3.7%) ⬇️
... and 71 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b56f09c...5097d8c. Read the comment docs.

codecov bot commented Nov 15, 2017

Codecov Report

Merging #3508 into master will increase coverage by 0.67%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3508      +/-   ##
==========================================
+ Coverage   34.61%   35.28%   +0.67%     
==========================================
  Files         261      267       +6     
  Lines        6769     6745      -24     
  Branches     1231     1221      -10     
==========================================
+ Hits         2343     2380      +37     
+ Misses       3733     3688      -45     
+ Partials      693      677      -16
Impacted Files Coverage Δ
blocks/url-input/index.js 3.17% <0%> (-0.06%) ⬇️
blocks/library/shortcode/index.js 40% <0%> (-10%) ⬇️
blocks/library/verse/index.js 28.57% <0%> (-8.93%) ⬇️
blocks/library/code/index.js 50% <0%> (-7.15%) ⬇️
blocks/library/html/index.js 16.66% <0%> (-6.42%) ⬇️
blocks/library/preformatted/index.js 44.44% <0%> (-5.56%) ⬇️
blocks/library/table/index.js 36.36% <0%> (-5.31%) ⬇️
blocks/library/text-columns/index.js 28.57% <0%> (-4.77%) ⬇️
blocks/library/video/index.js 16.66% <0%> (-4.39%) ⬇️
blocks/api/validation.js 91.46% <0%> (-3.7%) ⬇️
... and 71 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b56f09c...5097d8c. Read the comment docs.

Show outdated Hide outdated blocks/url-input/index.js Outdated
@vladanost

This comment has been minimized.

Show comment
Hide comment
@vladanost

vladanost Nov 20, 2017

Contributor

@aduth Thank you for catching that. I have refactored the code to do the same when pressing Enter key.

Contributor

vladanost commented Nov 20, 2017

@aduth Thank you for catching that. I have refactored the code to do the same when pressing Enter key.

@aduth

aduth approved these changes Nov 21, 2017

This looks to be a good intermediary fix, but I'm generally concerned about the behavior of the visible suggestions list being tied to a selection more than it is to input focus. There's also several places within URLInput we manage this: in input handlers, change handlers, selection handlers. Could do for some more refactoring I think, but fine for subsequent pull requests.

@aduth aduth merged commit 359f11f into WordPress:master Nov 21, 2017

2 checks passed

codecov/project 35.28% (+0.67%) compared to b56f09c
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment